On 8/22/26 21:14, Lorenzo Stoakes (ARM) wrote: > 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 <[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] > Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]>
Can we split off the selftest changes? This stable patch is already pretty big. I'd assume the changes to the selftests are not required just to get if fixed, because the changes should not be breaking existing user space (and cosnequently existing selftests). [...] > v1: > https://patch.msgid.link/[email protected] > > To: Andrew Morton <[email protected]> > To: Mike Rapoport <[email protected]> > To: David Hildenbrand <[email protected]> > To: "Liam R. Howlett" <[email protected]> > To: Vlastimil Babka <[email protected]> > To: Suren Baghdasaryan <[email protected]> > To: Michal Hocko <[email protected]> > To: Shuah Khan <[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: James Bottomley <[email protected]> > To: Hagen Paul Pfeifer <[email protected]> > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > --- [...] > + > +static bool secretmem_account_folio(struct secretmem_inode_state *state, > + const struct folio *folio) > +{ > + unsigned long nr_pages; Nit: const unsinged long nr_pages = folio_nr_pages(folio); > + > + 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; > + } > + Okay, that works because secretmem does not support any form of truncate, in particular, no FALLOC_FL_PUNCH_HOLE. Overall, the idea sounds good to me. Nothing jumped at me. -- Cheers, David
