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

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5383 bytes --]

Hi Lorenzo, Jann,

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

We use uppercase after the prefix, so s/add/Add/.

On Thu, Dec 05, 2024 at 10:41:25AM +0000, 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

The right amount of inter-sentence space is two.  See this:
<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/CONTRIBUTING.d/patches/description?id=bcf7d00fa4c7ce270f07d6e347c01b1f1e37580f>
<https://web.archive.org/web/20171217060354/http://www.heracliteanriver.com/?p=324>

> operations.
> 
> Reviewed-by: Jann Horn <jannh@google.com>

Thanks for the review!

> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Thanks for the patch!  I've applied it, with some minor tweaks.  See
comments below.
<https://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git/commit/?h=contrib&id=bb405ee3f6039226267fb1c6d2cb1fbb18d835bf>

Here's the diff that I applied when amending your patch:

diff --git i/man/man2/madvise.2 w/man/man2/madvise.2
index adb372424..fa24f6bf6 100644
--- i/man/man2/madvise.2
+++ w/man/man2/madvise.2
@@ -678,7 +678,8 @@ .SS Linux-specific advice values
 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
+Install a lightweight guard region
+into the range specified by
 .I addr
 and
 .IR size ,
@@ -686,22 +687,27 @@ .SS Linux-specific advice values
 .B SIGSEGV
 signal being raised.
 .IP
-If the region maps memory pages they will be cleared as part of the operation,
+If the region maps memory pages
+they will be cleared as part of the operation,
 though if
 .B MADV_GUARD_INSTALL
-is applied to regions containing pre-existing lightweight guard regions,
+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.
+This operation is supported
+only for writable anonymous private mappings
+which have not been mlock'd.
 An
 .B EINVAL
 error is returned if it is attempted on any other kind of mapping.
 .IP
 This operation is more efficient than mapping a new region of memory
 .BR PROT_NONE ,
-as it does not require the establishment of new mappings,
-instead regions of an existing mapping simply have their page tables
+as it does not require the establishment of new mappings.
+Instead,
+regions of an existing mapping
+simply have their page tables
 manipulated to establish the desired behavior.
 No additional memory is used.
 .IP
@@ -740,12 +746,15 @@ .SS Linux-specific advice values
 operation is applied to them.
 .TP
 .BR MADV_GUARD_REMOVE " (since Linux 6.13)"
-Remove any lightweight guard regions which exist in the range specified by
+Remove any lightweight guard regions
+which exist in the range specified by
 .I addr
 and
 .IR size .
 .IP
-All mappings in the range other than lightweight guard regions are left in place
+All mappings in the range
+other than lightweight guard regions
+are left in place
 (including mlock'd mappings).
 The operation is,
 however,

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

I've broken lines a little bit more, even though they were correct, just
for having shorter lines.

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

I missed this before.  It was the same misplacement of only.  :)

Have a lovely day!
Alex


-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]