Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings

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

On Tue, Feb 25, 2025 at 04:54:22PM +0100, Vlastimil Babka wrote:
> On 2/18/25 18:28, Lorenzo Stoakes wrote:
> > On Tue, Feb 18, 2025 at 06:25:35PM +0100, David Hildenbrand wrote:
> >>
> >> > > >
> >> > > > It fails because it tries to 'touch' the memory, but 'touching' guard
> >> > > > region memory causes a segfault. This kind of breaks the idea of
> >> > > > mlock()'ing guard regions.
> >> > > >
> >> > > > I think adding workarounds to make this possible in any way is not really
> >> > > > worth it (and would probably be pretty gross).
> >> > > >
> >> > > > We already document that 'mlock()ing lightweight guard regions will fail'
> >> > > > as per man page so this is all in line with that.
> >> > >
> >> > > Right, and I claim that supporting VM_LOCKONFAULT might likely be as easy as
> >> > > allowing install/remove of guard regions when that flag is set.
> >> >
> >> > We already allow this flag! VM_LOCKED and VM_HUGETLB are the only flags we
> >> > disallow.
> >>
> >>
> >> See mlock2();
> >>
> >> SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
> >> {
> >> 	vm_flags_t vm_flags = VM_LOCKED;
> >>
> >> 	if (flags & ~MLOCK_ONFAULT)
> >> 		return -EINVAL;
> >>
> >> 	if (flags & MLOCK_ONFAULT)
> >> 		vm_flags |= VM_LOCKONFAULT;
> >>
> >> 	return do_mlock(start, len, vm_flags);
> >> }
> >>
> >>
> >> VM_LOCKONFAULT always as VM_LOCKED set as well.
> >
> > OK cool, that makes sense.
> >
> > As with much kernel stuff, I knew this in the past. Then I forgot. Then I knew
> > again, then... :P if only somebody would write it down in a book...
> >
> > Yeah then that makes sense to check explicitly for (VM_LOCKED | VM_LOCKONFAULT)
> > in any MADV_GUARD_INSTALL_LOCKED variant as obviously this would be passively
> > excluded right now.
>
> Sorry for the late reply. So AFAIU from your conversations, guards can't be
> compatible with VM_LOCKED, which means e.g. any attempts of glibc to use
> guards for stacks will soon discover that mlockall() users exist and are
> broken by this, and the attempts will fail? That's a bummer.
>

Yeah damn, this pushes up the priority on this.

Yeah unfortunately we cannot support this with guard regions being installed
_after_ the mlockall() but can for before.

Let me write this on my eternal whiteboard of doom (TM), because that ups the
priority on this. I want to have a good think and see if it might after all be
possible to find a way to make things work here for sake of this case.

This thing is already shipped now, so it is inevitably going to be an add-on.

I will try some experiments when I get a sec.

Thanks very much for bringing this point up! This is pretty key.

> As for compatibility with VM_LOCKONFAULT, do we need a new
> MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
> that it can be just retrofitted (like you retrofit file backed mappings)?
> AFAIU the only risk would be breaking somebody that already relies on a
> failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
>
>

Hmm yeah I suppose. I guess just to be consistent with the other _LOCKED
variants? (which seem to be... undocumented at least in man pages :P, and yes I
realise this is me semi-volunteering to do that obviously...).

But on the other hand, we could also expand this if you and I see also Dave feel
this makes sense and wouldn't be confusing.

Agreed entirely that it'd be very very odd for a user to rely on that so I think
we'll be fine.

I shall return to this topic later, in the form of a series, probably!

Cheers!