[PATCH v2] mm/secretmem: properly account locked pages

[Lorenzo Stoakes (ARM)]

secretmem has a relatively laissez-faire attitude to accounting the folios
it allocates.

The intention is that the memory is treated as if it were mlock()'d and
thus is limited by the RLIMIT_MEMLOCK limit if the CAP_IPC_LOCK capability
is not in place (which broadly allows unlimited ranges of mlock()'d
memory).

The lifecycle for memfd accounting against this limit is - account on map,
unaccount on unmap but the lifecycle of memfd folios is allocate on fault,
deallocate on inode eviction.

This mismatch is problematic because the folios are unevictable and remain
so until the inode is evicted (set using mapping_set_unevictable()).

This is problematic as it eliminates usual mlock() semantics - mapping
folios then unmapping them does not clear their unevictable state, since it
depends on AS_UNEVICTABLE, not PG_mlocked.

A user can therefore easily work around the RLIMIT_MEMLOCK limit - simply
map then unmap and VmLck no longer counts the secretmem range (or more
involved - fork which also achieves the same thing).

Worse - they are not accounted in the process's RSS even if mapped again,
meaning the OOM killer won't know to kill the process.

A user without the CAP_IPC_LOCK capability can therefore repeatedly
map/unmap (or map/fork) and consume all available system memory with
unevictable folios and cause system instability.

A secretmem fd can be passed between processes and over fork so a
per-process limit simply does not make sense.

So follow the precedent set by io_uring, perf, skbuff, iommufd and xdp -
track the number of locked pages in user_struct->locked_vm.

Since the scope tracked is actually inode lifetime, the RLIMIT_MEMLOCK
applies per-user not per-process. Also given the change in scope it doesn't
make sense to bypass for users with CAP_IPC_LOCK, so remove it.

There is simply no reason to carry on marking the mapping as mlock()'d
since it's misleading and the lifecycle is now correctly handled, so remove
this too.

Additionally, fix the selftest which checks the limit as this now must
assert SIGBUS on limit violation on fault-in.

__secretmem_account_pages() is more or less a duplicate of the code that
io_uring etc. use, but since this is a bug fix that needs backporting,
defer any de-duplication efforts to a follow-up.

Reported-by: Daehyeon Ko <4ncienth@xxxxxxxxx>
Closes: https://lore.kernel.org/linux-mm/20260813225328.2010303-1-4ncienth@xxxxxxxxx/
Fixes: 1507f51255c9 ("mm: introduce memfd_secret system call to create "secret" memory areas")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
---
v2:
- Tried to put the commit message on a diet, as per Mike.
- Moved secretmem_init_inode_[priv, destroy]() nearer their callers as per
  Mike.
- Removed CAP_IPC_LOCK check as per discussion with Mike.
- Removed redundant GFP_KERNEL in kzalloc_obj() call as per Mike.
- Updated to consistently use goto err style in secretmem_file_create() as
  per Mike.
- Updated test_mlock_limit() to consistently use goto cleanup style as per
  Mike.
- Added a comment describing why we are capping the mlock limit in the
  secretmem tests, as per Mike.
- Set inode->i_priv to NULL after destroy.

v1:
https://patch.msgid.link/20260814-secretmem-accounting-v1-1-d2f8c677980b@xxxxxxxxxx

To: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
To: Mike Rapoport <rppt@xxxxxxxxxx>
To: David Hildenbrand <david@xxxxxxxxxx>
To: "Liam R. Howlett" <liam@xxxxxxxxxxxxx>
To: Vlastimil Babka <vbabka@xxxxxxxxxx>
To: Suren Baghdasaryan <surenb@xxxxxxxxxx>
To: Michal Hocko <mhocko@xxxxxxxx>
To: Shuah Khan <shuah@xxxxxxxxxx>
To: Alexei Starovoitov <ast@xxxxxxxxxx>
To: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
To: "David S. Miller" <davem@xxxxxxxxxxxxx>
To: Jakub Kicinski <kuba@xxxxxxxxxx>
To: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
To: John Fastabend <john.fastabend@xxxxxxxxx>
To: Stanislav Fomichev <sdf@xxxxxxxxxxx>
To: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
To: Hagen Paul Pfeifer <hagen@xxxxxxxx>
Cc: ljs@xxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
Cc: linux-mm@xxxxxxxxx
Cc: linux-kselftest@xxxxxxxxxxxxxxx
Cc: netdev@xxxxxxxxxxxxxxx
Cc: bpf@xxxxxxxxxxxxxxx
---
 include/linux/sched/user.h                |   3 +-
 mm/secretmem.c                            | 117 +++++++++++++++++++++++++++---
 tools/testing/selftests/mm/memfd_secret.c |  96 ++++++++++++++++++++++--
 3 files changed, 198 insertions(+), 18 deletions(-)

diff --git a/include/linux/sched/user.h b/include/linux/sched/user.h
index 4cc52698e214..8d7e5521f7cd 100644
--- a/include/linux/sched/user.h
+++ b/include/linux/sched/user.h
@@ -25,7 +25,8 @@ struct user_struct {
 
 #if defined(CONFIG_PERF_EVENTS) || defined(CONFIG_BPF_SYSCALL) || \
 	defined(CONFIG_NET) || defined(CONFIG_IO_URING) || \
-	defined(CONFIG_VFIO_PCI_ZDEV_KVM) || IS_ENABLED(CONFIG_IOMMUFD)
+	defined(CONFIG_VFIO_PCI_ZDEV_KVM) || IS_ENABLED(CONFIG_IOMMUFD) || \
+	defined(CONFIG_SECRETMEM)
 	atomic_long_t locked_vm;
 #endif
 #ifdef CONFIG_WATCH_QUEUE
diff --git a/mm/secretmem.c b/mm/secretmem.c
index d29865075b6e..b7c8b2057e81 100644
--- a/mm/secretmem.c
+++ b/mm/secretmem.c
@@ -18,6 +18,8 @@
 #include <linux/secretmem.h>
 #include <linux/set_memory.h>
 #include <linux/sched/signal.h>
+#include <linux/sched/user.h>
+#include <linux/cred.h>
 
 #include <uapi/linux/magic.h>
 
@@ -47,10 +49,70 @@ bool secretmem_active(void)
 	return !!atomic_read(&secretmem_users);
 }
 
+struct secretmem_inode_state {
+	struct user_struct	*user;
+	atomic_long_t		nr_pages_accounted;
+};
+
+static bool __secretmem_account_pages(struct user_struct *user,
+		unsigned long nr_pages)
+{
+	unsigned long page_limit, cur_pages, new_pages;
+
+	if (!nr_pages)
+		return true;
+
+	page_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
+
+	cur_pages = atomic_long_read(&user->locked_vm);
+	do {
+		new_pages = cur_pages + nr_pages;
+		if (new_pages > page_limit)
+			return false;
+	} while (!atomic_long_try_cmpxchg(&user->locked_vm,
+					  &cur_pages, new_pages));
+	return true;
+}
+
+static bool secretmem_account_folio(struct secretmem_inode_state *state,
+		const struct folio *folio)
+{
+	unsigned long nr_pages;
+
+	nr_pages = folio_nr_pages(folio);
+	if (!__secretmem_account_pages(state->user, nr_pages))
+		return false;
+
+	atomic_long_add(nr_pages, &state->nr_pages_accounted);
+	return true;
+}
+
+static void __secretmem_unaccount_pages(struct secretmem_inode_state *state,
+		unsigned long nr_pages)
+{
+	atomic_long_sub(nr_pages, &state->user->locked_vm);
+	atomic_long_sub(nr_pages, &state->nr_pages_accounted);
+}
+
+static void secretmem_unaccount_folio(struct secretmem_inode_state *state,
+		struct folio *folio)
+{
+	__secretmem_unaccount_pages(state, folio_nr_pages(folio));
+}
+
+static void secretmem_unaccount_all_folios(struct secretmem_inode_state *state)
+{
+	unsigned long nr_pages_accounted;
+
+	nr_pages_accounted = atomic_long_read(&state->nr_pages_accounted);
+	__secretmem_unaccount_pages(state, nr_pages_accounted);
+}
+
 static vm_fault_t secretmem_fault(struct vm_fault *vmf)
 {
 	struct address_space *mapping = vmf->vma->vm_file->f_mapping;
 	struct inode *inode = file_inode(vmf->vma->vm_file);
+	struct secretmem_inode_state *state = inode->i_private;
 	pgoff_t offset = vmf->pgoff;
 	gfp_t gfp = vmf->gfp_mask;
 	unsigned long addr;
@@ -72,8 +134,15 @@ static vm_fault_t secretmem_fault(struct vm_fault *vmf)
 			goto out;
 		}
 
+		if (!secretmem_account_folio(state, folio)) {
+			folio_put(folio);
+			ret = VM_FAULT_SIGBUS;
+			goto out;
+		}
+
 		err = set_direct_map_invalid_noflush(folio_page(folio, 0));
 		if (err) {
+			secretmem_unaccount_folio(state, folio);
 			folio_put(folio);
 			ret = vmf_error(err);
 			goto out;
@@ -82,6 +151,7 @@ static vm_fault_t secretmem_fault(struct vm_fault *vmf)
 		__folio_mark_uptodate(folio);
 		err = filemap_add_folio(mapping, folio, offset, gfp);
 		if (unlikely(err)) {
+			secretmem_unaccount_folio(state, folio);
 			/*
 			 * If a split of large page was required, it
 			 * already happened when we marked the page invalid
@@ -112,22 +182,30 @@ static const struct vm_operations_struct secretmem_vm_ops = {
 	.fault = secretmem_fault,
 };
 
+static void secretmem_destroy_inode_priv(struct inode *inode)
+{
+	struct secretmem_inode_state *state = inode->i_private;
+
+	secretmem_unaccount_all_folios(state);
+	free_uid(state->user);
+	kfree(state);
+	inode->i_private = NULL;
+}
+
 static int secretmem_release(struct inode *inode, struct file *file)
 {
 	atomic_dec(&secretmem_users);
+	secretmem_destroy_inode_priv(inode);
+
 	return 0;
 }
 
 static int secretmem_mmap_prepare(struct vm_area_desc *desc)
 {
-	const unsigned long len = vma_desc_size(desc);
-
 	if (!vma_desc_test_any(desc, VMA_SHARED_BIT, VMA_MAYSHARE_BIT))
 		return -EINVAL;
 
-	vma_desc_set_flags(desc, VMA_LOCKED_BIT, VMA_DONTDUMP_BIT);
-	if (!mlock_future_ok(desc->mm, /*is_vma_locked=*/ true, len))
-		return -EAGAIN;
+	vma_desc_set_flags(desc, VMA_DONTDUMP_BIT);
 	desc->vm_ops = &secretmem_vm_ops;
 
 	return 0;
@@ -187,20 +265,40 @@ static const struct inode_operations secretmem_iops = {
 
 static struct vfsmount *secretmem_mnt;
 
+static int secretmem_init_inode_priv(struct inode *inode)
+{
+	struct secretmem_inode_state *state;
+
+	state = kzalloc_obj(*state);
+	if (!state)
+		return -ENOMEM;
+
+	state->user = get_uid(current_user());
+	inode->i_private = state;
+	return 0;
+}
+
 static struct file *secretmem_file_create(unsigned long flags)
 {
 	struct file *file;
 	struct inode *inode;
 	const char *anon_name = "[secretmem]";
+	int err;
 
 	inode = anon_inode_make_secure_inode(secretmem_mnt->mnt_sb, anon_name, NULL);
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
+	err = secretmem_init_inode_priv(inode);
+	if (err)
+		goto err_free_inode;
+
 	file = alloc_file_pseudo(inode, secretmem_mnt, "secretmem",
 				 O_RDWR | O_LARGEFILE, &secretmem_fops);
-	if (IS_ERR(file))
-		goto err_free_inode;
+	if (IS_ERR(file)) {
+		err = PTR_ERR(file);
+		goto err_free_priv;
+	}
 
 	mapping_set_gfp_mask(inode->i_mapping, GFP_USER);
 	mapping_set_unevictable(inode->i_mapping);
@@ -215,10 +313,11 @@ static struct file *secretmem_file_create(unsigned long flags)
 	atomic_inc(&secretmem_users);
 
 	return file;
-
+err_free_priv:
+	secretmem_destroy_inode_priv(inode);
 err_free_inode:
 	iput(inode);
-	return file;
+	return ERR_PTR(err);
 }
 
 SYSCALL_DEFINE1(memfd_secret, unsigned int, flags)
diff --git a/tools/testing/selftests/mm/memfd_secret.c b/tools/testing/selftests/mm/memfd_secret.c
index aac4f795c327..26d2a4a527da 100644
--- a/tools/testing/selftests/mm/memfd_secret.c
+++ b/tools/testing/selftests/mm/memfd_secret.c
@@ -15,6 +15,8 @@
 #include <sys/resource.h>
 #include <sys/capability.h>
 
+#include <setjmp.h>
+#include <signal.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -22,6 +24,8 @@
 #include <stdio.h>
 #include <fcntl.h>
 
+#include <sys/mman.h>
+
 #include "kselftest.h"
 
 #define fail(fmt, ...) ksft_test_result_fail(fmt, ##__VA_ARGS__)
@@ -31,6 +35,11 @@
 #ifdef __NR_memfd_secret
 
 #define PATTERN	0x55
+/*
+ * Set 8 MiB as a reasonable mlock limit so users with unlimited or absurdly
+ * high RLIMIT_MEMLOCK don't cause us to overflow in the tests.
+ */
+#define MLOCK_LIMIT_CAP	(8UL << 20)
 
 static const int prot = PROT_READ | PROT_WRITE;
 static const int mode = MAP_SHARED;
@@ -39,6 +48,13 @@ static unsigned long page_size;
 static unsigned long mlock_limit_cur;
 static unsigned long mlock_limit_max;
 
+static sigjmp_buf fault_env;
+
+static void sigbus_handler(int sig)
+{
+	siglongjmp(fault_env, 1);
+}
+
 static int memfd_secret(unsigned int flags)
 {
 	return syscall(__NR_memfd_secret, flags);
@@ -57,10 +73,31 @@ static void test_file_apis(int fd)
 		pass("file IO is blocked as expected\n");
 }
 
-static void test_mlock_limit(int fd)
+/* GUP disallows automatic fault-in of secretmem, so do it manually. */
+static bool fault_in_secretmem(char *mem, size_t len)
+{
+	if (sigsetjmp(fault_env, 1))
+		return false;
+	memset(mem, PATTERN, len);
+	return true;
+}
+
+static void test_mlock_limit(void)
 {
 	size_t len;
 	char *mem;
+	int fd;
+
+	/* Locked pages have an inode lifetime, so need a new fd. */
+	fd = memfd_secret(0);
+	if (fd < 0) {
+		fail("memfd_secret failed: %s\n", strerror(errno));
+		return;
+	}
+	if (ftruncate(fd, mlock_limit_max * 2)) {
+		fail("ftruncate failed: %s\n", strerror(errno));
+		goto out_close;
+	}
 
 	len = mlock_limit_cur;
 	if (len % page_size != 0)
@@ -69,19 +106,50 @@ static void test_mlock_limit(int fd)
 	mem = mmap(NULL, len, prot, mode, fd, 0);
 	if (mem == MAP_FAILED) {
 		fail("unable to mmap secret memory\n");
-		return;
+		goto out_close;
+	}
+
+	if (!fault_in_secretmem(mem, len)) {
+		fail("unable to fault in secret memory\n");
+		goto out;
 	}
-	munmap(mem, len);
 
+	munmap(mem, len);
 	len = mlock_limit_max * 2;
 	mem = mmap(NULL, len, prot, mode, fd, 0);
-	if (mem != MAP_FAILED) {
-		fail("unexpected mlock limit violation\n");
-		munmap(mem, len);
-		return;
+
+	if (mem == MAP_FAILED) {
+		fail("unable to mmap secret memory\n");
+		goto out_close;
+	}
+
+	if (fault_in_secretmem(mem, len)) {
+		fail("mlock limit is not respected\n");
+		goto out;
+	}
+
+	munmap(mem, len);
+	len = page_size;
+
+	/* map a page past the limit to assert inode scope. */
+
+	mem = mmap(NULL, page_size, prot, mode, fd,
+		   mlock_limit_max & ~(page_size - 1));
+	if (mem == MAP_FAILED) {
+		fail("unable to mmap secret memory\n");
+		goto out_close;
+	}
+
+	if (fault_in_secretmem(mem, page_size)) {
+		fail("mlock limit is not respected\n");
+		goto out;
 	}
 
 	pass("mlock limit is respected\n");
+out:
+	munmap(mem, len);
+out_close:
+	close(fd);
 }
 
 static void test_vmsplice(int fd, const char *desc)
@@ -292,6 +360,12 @@ static void prepare(void)
 	if (page_size > mlock_limit_max)
 		mlock_limit_max = page_size;
 
+	/* Clamp huge or unlimited. */
+	if (mlock_limit_max > MLOCK_LIMIT_CAP)
+		mlock_limit_max = MLOCK_LIMIT_CAP;
+	if (mlock_limit_cur > mlock_limit_max)
+		mlock_limit_cur = mlock_limit_max;
+
 	if (set_cap_limits(mlock_limit_max))
 		ksft_exit_fail_msg("Unable to set mlock limit: %s\n",
 				   strerror(errno));
@@ -301,6 +375,7 @@ static void prepare(void)
 
 int main(int argc, char *argv[])
 {
+	struct sigaction sa = { .sa_handler = sigbus_handler };
 	int fd;
 
 	prepare();
@@ -316,10 +391,15 @@ int main(int argc, char *argv[])
 			ksft_exit_fail_msg("memfd_secret failed: %s\n",
 					   strerror(errno));
 	}
+
+	sigemptyset(&sa.sa_mask);
+	if (sigaction(SIGBUS, &sa, NULL))
+		ksft_exit_fail_msg("Cannot set up SIGBUS handler");
+
 	if (ftruncate(fd, page_size))
 		ksft_exit_fail_msg("ftruncate failed: %s\n", strerror(errno));
 
-	test_mlock_limit(fd);
+	test_mlock_limit();
 	test_file_apis(fd);
 	/*
 	 * We have to run the first vmsplice test before any secretmem page was

---
base-commit: e737cebb8de0d38e8f64584a8bbfbcf9176c7537
change-id: 20260814-secretmem-accounting-ad6a44629b19

Cheers,
-- 
Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>