Re: [PATCH man-pages v4] madvise.2: add MADV_GUARD_INSTALL, MADV_GUARD_REMOVE description

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

On Thu, Dec 05, 2024 at 06:50:09PM +0100, Vlastimil Babka wrote:
> On 12/5/24 11:41, Lorenzo Stoakes wrote:
> > Lightweight guard region support has been added to Linux 6.13, which adds
> > MADV_GUARD_INSTALL and MADV_GUARD_REMOVE flags to the madvise() system
> > call. Therefore, update the manpage for madvise() and describe these
> > operations.
> >
> > Reviewed-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> > v4:
> > * Reference function chapters as per Alejandro.
> > * Minor rewording as per Alejandro.
> >
> > v3:
> > * Don't describe SIGSEGV as a fatal signal as per Jann.
> > https://lore.kernel.org/all/20241202165829.72121-1-lorenzo.stoakes@oracle.com
> >
> > v2:
> > * Updated to use semantic newlines as suggested by Alejandro.
> > * Avoided emboldening parens as suggested by Alejandro.
> > * One very minor grammatical fix.
> > https://lore.kernel.org/all/20241129155943.85215-1-lorenzo.stoakes@oracle.com
> >
> > v1:
> > https://lore.kernel.org/all/20241129093205.8664-1-lorenzo.stoakes@oracle.com
> >
> >  man/man2/madvise.2 | 93 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> >
> > diff --git a/man/man2/madvise.2 b/man/man2/madvise.2
> > index 4f2210ee2..7d682fa40 100644
> > --- a/man/man2/madvise.2
> > +++ b/man/man2/madvise.2
> > @@ -676,6 +676,91 @@ or secret memory regions created using
> >  Note that with
> >  .BR MADV_POPULATE_WRITE ,
> >  the process can be killed at any moment when the system runs out of memory.
> > +.TP
> > +.BR MADV_GUARD_INSTALL " (since Linux 6.13)"
> > +Install a lightweight guard region into the range specified by
> > +.I addr
> > +and
> > +.IR size ,
> > +causing any read or write in the range to result in a
> > +.B SIGSEGV
> > +signal being raised.
> > +.IP
> > +If the region maps memory pages they will be cleared as part of the operation,
> > +though if
>
> Hm this reads a bit ambiguous. One could read it as the memory pages are
> being cleared, but it's the page tables.

This was really hard to word, because you don't want to say unmapped, and saying
'clearing page tables' or 'zapping' is clear to us but not necessarily to a
reader. 'Clearing mapping' makes it ambiguous vs. munmap(), etc. etc.

But you want to make it clear (no pun intended) that anon pages, if there's any
data, it's probably lost. So I think this is a distinction that doesn't matter.

Will revisit once we support file-backed mappings.

>
> > +.B MADV_GUARD_INSTALL
> > +is applied to regions containing pre-existing lightweight guard regions,
> > +they are left in place.
> > +.IP
> > +This operation is only supported for writable anonymous private mappings which
> > +have not been mlock'd.
>
> Not sure if "mlock'd" is the canonical term, I think I've seen "locked" used
> before, which I don't think it's great. Maybe Alejandro knows better.
>
> (there's also another "mlock'd" later in the patch)
>

There's many different ways of saying this it's not really hugely
consistent. I've seen mlock'ed mlock'd mlock()ed etc. etc.

So there is no 'canonical' term, and I think it's pretty clear what is meant
here. There's also a reference to "mlock'd" earlier in the file (though in a
comment...)