Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] seccomp_unotify.2: Add doc for SECCOMP_ADDFD_FLAG_SEND
From:   "Alejandro Colomar (man-pages)" <alx.manpages@gmail.com>
To:     Rodrigo Campos <rodrigo@kinvolk.io>
Cc:     linux-kernel@vger.kernel.org, Alban Crequy <alban@kinvolk.io>,
        =?UTF-8?Q?Mauricio_V=c3=a1squez_Bernal?= <mauricio@kinvolk.io>,
        Sargun Dhillon <sargun@sargun.me>, linux-man@vger.kernel.org,
        Michael Kerrisk <mtk.manpages@gmail.com>
References: <20210702163744.265749-1-rodrigo@kinvolk.io>
 <6322ac90-3491-df4f-7505-db4081bacb79@gmail.com>
Message-ID: <82b7b6a4-14bf-23d9-dad9-f348fe905d8e@gmail.com>
Date:   Sun, 4 Jul 2021 13:25:51 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <6322ac90-3491-df4f-7505-db4081bacb79@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/3/21 11:25 PM, Alejandro Colomar (man-pages) wrote:
> Hi Rodrigo,
> 
> On 7/2/21 6:37 PM, Rodrigo Campos wrote:
>> This flag was recently added to Linux 5.14 by a patch I wrote:
>> 	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0ae71c7720e3ae3aabd2e8a072d27f7bd173d25c
>>
>> This patch adds documentation for the flag, the error code that the flag
>> added and explains in the caveat when it is useful.
>>
>> Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
>> ---
>> Hi! Here goes the documentation for the flag I just added. Please feel free to
>> amend as you want and let me know if something is not clear :)
> 
> Thanks for documenting your own addition!
> That makes things much easier :-)
> 
> It looks quite good to me.
> 
> There are a few minor changes that I applied in a following patch.  I'll
> explain why in your patch inline.  And then you have the diff below your

And I meant: patch applied!

Thanks,

Alex

> patch.
> 
> Cheers,
> 
> Alex
> 
>>
>>
>>  man2/seccomp_unotify.2 | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/man2/seccomp_unotify.2 b/man2/seccomp_unotify.2
>> index 2673d9bc7..9bd27214f 100644
>> --- a/man2/seccomp_unotify.2
>> +++ b/man2/seccomp_unotify.2
>> @@ -739,6 +739,17 @@ When allocating the file descriptor in the target,
>>  use the file descriptor number specified in the
>>  .I newfd
>>  field.
>> +.TP
>> +.BR SECCOMP_ADDFD_FLAG_SEND
>> +Available since Linux 5.14, combines the
> 
> We usually append that info to the paragraph tag (i.e., the line just
> after .TP), and with a common syntax, so that it's easier to read..
> 
>> +.B SECCOMP_IOCTL_NOTIF_ADDFD
>> +ioctl with
>> +.B SECCOMP_IOCTL_NOTIF_SEND
>> +into an atomic operation. On successful invocation, the target process's
>> +errno will be 0 and the return value will be the file descriptor number that was
>> +installed in the target. If allocating the file descriptor in the tatget fails,
>> +the target's syscall continues to be blocked until a successful response is
>> +sent.
> 
> See the following extract from man-pages(7):
> 
> $ man 7 man-pages | sed -n '/Use semantic newlines/,/^$/p';
>    Use semantic newlines
>        In the source of a manual page,  new  sentences  should  be
>        started  on new lines, and long sentences should split into
>        lines at clause breaks (commas, semicolons, colons, and  so
>        on).   This  convention,  sometimes known as "semantic new‐
>        lines", makes it easier to see the effect of patches, which
>        often  operate at the level of individual sentences or sen‐
>        tence clauses.
> 
>>  .RE
>>  .TP
>>  .I srcfd
>> @@ -801,6 +812,13 @@ Allocating the file descriptor in the target would cause the target's
>>  limit to be exceeded (see
>>  .BR getrlimit (2)).
>>  .TP
>> +.B EBUSY
>> +If the flag
>> +.B SECCOMP_IOCTL_NOTIF_SEND
>> +is used, this means the operation can't proceed until other
>> +.B SECCOMP_IOCTL_NOTIF_ADDFD
>> +requests are processed.
>> +.TP
>>  .B EINPROGRESS
>>  The user-space notification specified in the
>>  .I id
>> @@ -1131,6 +1149,14 @@ that would
>>  normally be restarted by the
>>  .BR SA_RESTART
>>  flag.
>> +.PP
>> +Furthermore, if the supervisor response is a file descriptor
>> +added with
>> +.B SECCOMP_IOCTL_NOTIF_ADDFD,
>> +then the flag
>> +.B SECCOMP_ADDFD_FLAG_SEND
>> +can be used to atomically add the file descriptor and return that value,
>> +making sure no file descriptors are inadvertently leaked into the target.
> 
> I moved your paragraph below the FIXME, as I the FIXME applies to the
> previous paragraph ("About the above").
> 
>>  .\" FIXME
>>  .\" About the above, Kees Cook commented:
>>  .\"
>>
> 
> 
> diff --git a/man2/seccomp_unotify.2 b/man2/seccomp_unotify.2
> index 9bd27214f..ae449ae36 100644
> --- a/man2/seccomp_unotify.2
> +++ b/man2/seccomp_unotify.2
> @@ -740,16 +740,18 @@ use the file descriptor number specified in the
>  .I newfd
>  field.
>  .TP
> -.BR SECCOMP_ADDFD_FLAG_SEND
> -Available since Linux 5.14, combines the
> +.BR SECCOMP_ADDFD_FLAG_SEND " (since Linux 5.14)"
> +Combines the
>  .B SECCOMP_IOCTL_NOTIF_ADDFD
>  ioctl with
>  .B SECCOMP_IOCTL_NOTIF_SEND
> -into an atomic operation. On successful invocation, the target process's
> -errno will be 0 and the return value will be the file descriptor number
> that was
> -installed in the target. If allocating the file descriptor in the
> tatget fails,
> -the target's syscall continues to be blocked until a successful response is
> -sent.
> +into an atomic operation.
> +On successful invocation, the target process's errno will be 0
> +and the return value will be the file descriptor number
> +that was installed in the target.
> +If allocating the file descriptor in the tatget fails,
> +the target's syscall continues to be blocked
> +until a successful response is sent.
>  .RE
>  .TP
>  .I srcfd
> @@ -1149,14 +1151,6 @@ that would
>  normally be restarted by the
>  .BR SA_RESTART
>  flag.
> -.PP
> -Furthermore, if the supervisor response is a file descriptor
> -added with
> -.B SECCOMP_IOCTL_NOTIF_ADDFD,
> -then the flag
> -.B SECCOMP_ADDFD_FLAG_SEND
> -can be used to atomically add the file descriptor and return that value,
> -making sure no file descriptors are inadvertently leaked into the target.
>  .\" FIXME
>  .\" About the above, Kees Cook commented:
>  .\"
> @@ -1176,6 +1170,14 @@ making sure no file descriptors are inadvertently
> leaked into the target.
>  .\" calls because it's impossible for the kernel to restart the call
>  .\" with the right timeout value. I wonder what happens when those
>  .\" system calls are restarted in the scenario we're discussing.)
> +.PP
> +Furthermore, if the supervisor response is a file descriptor
> +added with
> +.B SECCOMP_IOCTL_NOTIF_ADDFD,
> +then the flag
> +.B SECCOMP_ADDFD_FLAG_SEND
> +can be used to atomically add the file descriptor and return that value,
> +making sure no file descriptors are inadvertently leaked into the target.
>  .SH BUGS
>  If a
>  .BR SECCOMP_IOCTL_NOTIF_RECV
> 
> 

-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/