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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, May 20, 2025 at 12:41:00PM +0100, Pedro Falcato wrote:
> 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.

Yeah sounds fair, will do on respin!

>
> > >
> > > 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.

Yeah perhaps with _ALL_ thrown in to make this clear... :) warming to it... ;)

>
> --
> Pedro