Re: [RFC PATCH 4/5] mm/madvise: add PMADV_SET_FORK_EXEC_DEFAULT process_madvise() flag

[Pedro Falcato <pfalcato@suse.de>]

On Tue, May 20, 2025 at 11:21:33AM +0100, Lorenzo Stoakes wrote:
> On Tue, May 20, 2025 at 09:38:50AM +0100, Pedro Falcato wrote:
> > On Mon, May 19, 2025 at 09:52:41PM +0100, Lorenzo Stoakes wrote:
> > > It's useful in certain cases to be able to default-enable an madvise() flag
> > > for all newly mapped VMAs, and for that to survive fork/exec.
> > >
> > > The natural place to specify something like this is in an madvise()
> > > invocation, and thus providing this functionality as a flag to
> > > process_madvise() makes sense.
> > >
> > > We intentionally limit this only to flags that we know should function
> > > correctly without issue, and to be conservative about this, so we initially
> > > limit ourselves only to MADV_HUGEPAGE, MADV_NOHUGEPAGE, that is - setting
> > > the VM_HUGEPAGE, VM_NOHUGEPAGE VMA flags.
> > >
> > > We implement this functionality by using the mm_struct->def_flags field.
> >
> > This seems super specific. How about this:
> >
> > - PMADV_FUTURE (mirrors MCL_FUTURE). This only applies the flag to future VMAs in the current process.
> > - PMADV_INHERIT_FORK. This makes it so the flag is propagated to child processes (does not imply PMADV_FUTURE)
> > - PMADV_INHERIT_EXEC. This makes it so the flag is propagated through the execve boundary
> >   (and this is where we'd filter for 'safe' flags, at least through the secureexec boundary). Does not imply
> >   FUTURE nor INHERIT_FORK.
> 
> I don't know how we could implement separate current process, fork, exec, fork/exec.
> mm->def_flags is propagated this way automatically.
> 
> And again on the security stuff, I think the correct answer is to require sys
> admin capability to be able to use this option _at all_. This simplifies
> everything.
> 
> To have this kind of thing we'd have to add a whole new mechanism, literally
> just for this, and I'd really rather not generate brand new mm_struct flags for
> every possible mode (in fact that would probably makes the whole thing
> intractible), or add a new field there for this.
> 
> The idea is that we get the advantages of an improved madvise interface, while
> also providing the interface Usama wants without having to add some hideous
> prctl() whose logic is disconnected from the rest of madvise(), while being, in
> effect, a 'default madvise() for new mappings'.
> 
> So while specific to the case, nothing prevents us in future adding more
> functionality if we want.
> 
> We could also potentially:
> 
> - add PMADV_SET_DEFAULT (I'm iffy about PMADV_FUTURE... but whichever we go with)
> - add PMADV_INHERIT_FORK
> - add PMADV_INHERIT_EXEC
> 
> And only support PMADV_SET_DEFAULT | PMADV_INHERIT_FORK | PMADV_INHERIT_EXEC for
> now.
>
> THen we could have the security semantics you specify (require cap sys admin on
> PMADV_INHERIT_EXEC) but have that propagate to the only supported case.
> 
> What do you think?
>

If you don't want to add new fields, this option seems fine.
And then if any other usecase pops up, we're ready.
 
> >
> > and, while we're at it, rename PMADV_ENTIRE_ADDRESS_SPACE to PMADV_CURRENT, to align it with MCL_CURRENT.
> 
> I'm not sure making the mlock()/madvise() stuff analagous is a good idea, as
> they have different semantics. I'd rather keep these flags descriptive. Though
> I'm open to alternative naming of course...

Semantics are similar I think? And I do think getting shorter names is a good
idea, however I won't insist too hard on this.

-- 
Pedro