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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3102ab3b-67b6-4047-9db7-e83c3b9c1c53@lucifer.local>
Date: Tue, 25 Feb 2025 16:37:40 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Matthew Wilcox <willy@...radead.org>,
        "Paul E . McKenney" <paulmck@...nel.org>, Jann Horn <jannh@...gle.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        linux-api@...r.kernel.org, John Hubbard <jhubbard@...dia.com>,
        Juan Yescas <jyescas@...gle.com>,
        Kalesh Singh <kaleshsingh@...gle.com>
Subject: Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem
 mappings

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!