On Tue, Aug 25, 2026 at 12:50:57PM +0200, David Hildenbrand (Arm) wrote: > 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.
Yeah I can do! I seem to recall people wanting tests backported too hence the change. > > 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). Yup confirmed locally that the changes don't break the tests. > > [...] > > > 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); Ack > > > + > > + 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. Yeah exactly. I think I covered that off somewhere in my essay-length commit msg but if not but yeah that is a thing that I noted. > > Overall, the idea sounds good to me. Nothing jumped at me. Thanks! So in a way you kinda... Ack it right? :P If only there were a tag for that 🤔 ;) > > -- > Cheers, > > David -- Cheers, Lorenzo
