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

[Mike Rapoport]

> 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.
> 
> This is established using mapping_set_unevictable() setting the
> AS_UNEVICTABLE flag in the relevant address_space data structure.
> 
> 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.
> 
> This is checked by folio_evictable() which first checks for AS_UNEVICTABLE
> before looking at PG_mlocked, which is cleared on unmap via
> munlock_vma_folio().
> 
> A user can therefore easily work around the RLIMIT_MEMLOCK limit - simply
> map then unmap and VmLck no longer counts the secretmem range, nor are they
> accounted in the process's RSS even if mapped again, meaning the OOM killer
> won't know to kill the process.
> 
> This can also be achieved by forking the process, as VMA_LOCKED_BIT is
> cleared for VMAs copied to the child process, whose VmLck will be 0.
> 
> 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.
> 
> Worse, the OOM killer will not be able to resolve the situation.
> 
> Additionally, this fd can be passed between processes and over fork so a
> per-process limit simply does not make sense.
> 
> There is precedent for addressing this issue - io_uring, perf, skbuff,
> iommufd and xdp all perform the precise lifetime management required here
> using an alternative method - tracking the number of locked pages in
> struct user_struct->locked_vm.
> 
> Add secretmem to this list and account locked folios over the lifetime of
> the inode to reflect the actual lifetime of the folios, while bypassing
> this if the user has the CAP_IPC_LOCK capability.
> 
> As a result the semantics change - the RLIMIT_MEMLOCK limit is per-user not
> per-process to reflect the actual scope of the allocated folios, and this
> limit is shared between secretmem and io_uring, etc.
> 
> However this is reasonable given the need to track this limit in the actual
> scope in which it applies.
> 
> Since GUP rejects secretmem mappings, setting VMA_LOCKED_BIT does not
> result in memory being faulted in on map, another wrinkle with the mlock()
> accounting.
> 
> This also leads to another oddity from the previous implementation -
> mlock_future_ok() was checked on mmap() but since nothing is faulted in
> right away, this check was more or less meaningless. Therefore drop this.
> 
> 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.
> 
> Also assert there that the limit applies to the lifetime of the fd rather
> than the mapping by trying to map a single page past the maximum rlimit -
> previously this would have succeeded as the prior unmap would have reset
> the mlock limit.
> 
> __secretmem_account_pages() is essentially 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.

Can the commit message here be a page rather than a folio? ;-)

> 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>
>
> 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..537fe5b1222f 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,107 @@ 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;
> +
> +	if (!state)
> +		return true;
> +
> +	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)
> +{
> +	if (!state)
> +		return;
> +
> +	__secretmem_unaccount_pages(state, folio_nr_pages(folio));
> +}
> +
> +static void secretmem_unaccount_all_folios(struct secretmem_inode_state *state)
> +{
> +	unsigned long nr_pages_accounted;
> +
> +	if (!state)
> +		return;
> +
> +	nr_pages_accounted = atomic_long_read(&state->nr_pages_accounted);
> +	__secretmem_unaccount_pages(state, nr_pages_accounted);
> +}
> +
> +static void secretmem_destroy_inode_priv(struct inode *inode)
> +{
> +	struct secretmem_inode_state *state = inode->i_private;
> +
> +	if (!state)
> +		return;
> +
> +	secretmem_unaccount_all_folios(state);
> +	free_uid(state->user);
> +	kfree(state);
> +}
> +
> +static int secretmem_init_inode_priv(struct inode *inode)

Can _init and _destroy live closer to their callers please?

> +{
> +	struct secretmem_inode_state *state;
> +
> +	if (ns_capable_noaudit(&init_user_ns, CAP_IPC_LOCK))
> +		return 0;

The check happens only on inode creation, so if a privileged process,
e.g. a container engine, passes the fd to an unprivileged process, the
accounting issue remains.

> +
> +	state = kzalloc_obj(*state, GFP_KERNEL);

GFP_KERNEL is implicit for kzalloc_obj(), isn't it?

> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->user = get_uid(current_user());
> +	inode->i_private = state;
> +	return 0;
> +}
> +
>  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 +171,15 @@ static vm_fault_t secretmem_fault(struct vm_fault *vmf)
>  			goto out;
>  		}
>  
> +		if (!secretmem_account_folio(state, folio)) {

Oh, this is getting really hairy :(
We need better gotos in secretmem_fault(), it could be a nice
preparation patch, but I don't feel very strongly about it.

> +			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 +188,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
> @@ -115,19 +222,17 @@ static const struct vm_operations_struct secretmem_vm_ops = {
>  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;
> @@ -192,15 +297,23 @@ 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))
> +	if (IS_ERR(file)) {
> +		secretmem_destroy_inode_priv(inode);

Please use goto for error handling here.

> +		err = PTR_ERR(file);
>  		goto err_free_inode;
> +	}
>  
>  	mapping_set_gfp_mask(inode->i_mapping, GFP_USER);
>  	mapping_set_unevictable(inode->i_mapping);
> @@ -218,7 +331,7 @@ static struct file *secretmem_file_create(unsigned long flags)
>  
>  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..626e7033b72f 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,7 @@
>  #ifdef __NR_memfd_secret
>  
>  #define PATTERN	0x55
> +#define MLOCK_LIMIT_CAP	(8UL << 20)

A comment about why capping at 8MB would be nice.

>  static const int prot = PROT_READ | PROT_WRITE;
>  static const int mode = MAP_SHARED;
> @@ -39,6 +44,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 +69,32 @@ 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;
> +	}

Can't we ftruncate to this size in main and be done?

>  
>  	len = mlock_limit_cur;
>  	if (len % page_size != 0)
> @@ -69,19 +103,48 @@ 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)) {
> +		munmap(mem, len);
> +		fail("unable to fault in secret memory\n");
> +		goto out_close;

Please don't mix cleanup styles, goto err_do_cleanup is the preferred
way.

-- 
Sincerely yours,
Mike.