secretmem accounts folios by treating memory as if it were mlock()'d and thus limited by the RLIMIT_MEMLOCK limit.
However the folios are unevictable and remain so until the inode is evicted, eliminating 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. Worse, folios are not accounted in the process's RSS, meaning the OOM killer won't know to kill the process. Repeatedly mapping/unmapping (or forking) can then result in the consumption of 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 by tracking 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, so it doesn't make sense to bypass for users with CAP_IPC_LOCK, therefore remove this bypass. 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. Note that secretmem does not support any form of truncation (including hole punching) and the folios are unreclaimable, so the folios need only be accounted on fault and unaccounted on inode destruction. __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. test_mlock_limit() asserts mlock_future_ok() on mmap(), however this has been removed, so remove the test altogether for the fix. A new test will be sent separately for upstream. Reported-by: Daehyeon Ko <[email protected]> Closes: https://lore.kernel.org/linux-mm/[email protected]/ Fixes: 1507f51255c9 ("mm: introduce memfd_secret system call to create "secret" memory areas") Cc: [email protected] Reviewed-by: Mike Rapoport (Microsoft) <[email protected]> Acked-by: David Hildenbrand (Arm) <[email protected]> Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]> --- v3: - Accumulated tags (thanks Mike and David!) - Tried to cut down commit message further as per Mike. - Made local vars const as per David. - Removed test (will send separately) as per David. - Added comment about hole punching and truncation as per David. - Some small commit message fixups. 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. https://patch.msgid.link/[email protected] v1: https://patch.msgid.link/[email protected] To: Andrew Morton <[email protected]> To: Mike Rapoport <[email protected]> To: Alexei Starovoitov <[email protected]> To: Daniel Borkmann <[email protected]> To: "David S. Miller" <[email protected]> To: Jakub Kicinski <[email protected]> To: Jesper Dangaard Brouer <[email protected]> To: John Fastabend <[email protected]> To: Stanislav Fomichev <[email protected]> To: Hagen Paul Pfeifer <[email protected]> To: James Bottomley <[email protected]> To: "Liam R. Howlett" <[email protected]> To: Vlastimil Babka <[email protected]> To: Suren Baghdasaryan <[email protected]> To: Michal Hocko <[email protected]> Cc: "David Hildenbrand (Arm)" <[email protected]> Cc: [email protected] Cc: [email protected] Cc: [email protected] Cc: [email protected] Cc: [email protected] Cc: [email protected] --- include/linux/sched/user.h | 3 +- mm/secretmem.c | 116 +++++++++++++++++++++++++++--- tools/testing/selftests/mm/memfd_secret.c | 30 +------- 3 files changed, 110 insertions(+), 39 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..384f5cfc457f 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,69 @@ 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) +{ + const unsigned long 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) +{ + const unsigned long 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 +133,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 +150,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 +181,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 +264,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 +312,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..c55d84c5e613 100644 --- a/tools/testing/selftests/mm/memfd_secret.c +++ b/tools/testing/selftests/mm/memfd_secret.c @@ -57,33 +57,6 @@ static void test_file_apis(int fd) pass("file IO is blocked as expected\n"); } -static void test_mlock_limit(int fd) -{ - size_t len; - char *mem; - - len = mlock_limit_cur; - if (len % page_size != 0) - len = (len/page_size) * page_size; - - mem = mmap(NULL, len, prot, mode, fd, 0); - if (mem == MAP_FAILED) { - fail("unable to mmap secret memory\n"); - return; - } - 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; - } - - pass("mlock limit is respected\n"); -} - static void test_vmsplice(int fd, const char *desc) { ssize_t transferred; @@ -297,7 +270,7 @@ static void prepare(void) strerror(errno)); } -#define NUM_TESTS 6 +#define NUM_TESTS 5 int main(int argc, char *argv[]) { @@ -319,7 +292,6 @@ int main(int argc, char *argv[]) if (ftruncate(fd, page_size)) ksft_exit_fail_msg("ftruncate failed: %s\n", strerror(errno)); - test_mlock_limit(fd); 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) <[email protected]>
