Re: rseq(2) man page

[Mathieu Desnoyers]

On 2023-01-06 12:50, Alejandro Colomar wrote:
> Hi Mathieu,
>
> See some comments below.
>
> Cheers,
>
> Alex

[...]

> We now use SPDX-License-Identifier.

OK

> > .\"
> > .TH RSEQ 2 2020-06-05 "Linux" "Linux Programmer's Manual"
>
> We use lowercase for the function name (or to be more precise, the same 
> case that the function name uses.
>
> The date is specified with a placeholder that is replaced at the time of 
> creation of the tarball.
>
> The 5th argument is unspecified.
>
> The 4th argument is now the project name and a placeholder for the version.
>
> See an example:
>
> $ cat man2/membarrier.2 | grep '^.TH'
> .TH membarrier 2 (date) "Linux man-pages (unreleased)"

OK

> > .SH NAME
> > rseq \- Restartable sequences system call
>
> We use lowercase here, so s/Restartable/restartable/

OK

> > .SH SYNOPSIS
> > .nf
> > .B #include <linux/rseq.h>
>
> Is there a glibc wrapper for this syscall?  Do you expect that it will 
> be added relatively soon?  Or is it expected that this syscall will be 
> called through syscall(2) for many years?

The system call is only for registering/unregistering the per-thread 
area, so I expect it will only be used through syscall(2), and typically 
only from libc.

User applications/libraries are expected to use the per-thread area to 
communicate with the kernel, but that does not require use of rseq(2).

> If so, it may be better to document it directly as such, like for 
> example membarrier:
>
> SYNOPSIS
>         #include <linux/membarrier.h> /* Definition of MEMBARRIER_* 
> constants */
>         #include <sys/syscall.h>      /* Definition of SYS_* constants */
>         #include <unistd.h>
>
>         int syscall(SYS_membarrier, int cmd, unsigned int flags, int 
> cpu_id);
>
>         Note: glibc provides no wrapper for membarrier(), necessitating 
> the use
>         of syscall(2).

OK

> > .sp
>
> s/sp/PP/

OK

> > .BI "int rseq(struct rseq * " rseq ", uint32_t " rseq_len ", int " 
> > flags ", 
>
> Is it valid to pass NULL in 'rseq'?  If it is, we now document that 
> using _Nullable.  See for example recv(2):
>
> SYNOPSIS
>         #include <sys/socket.h>
>
>         ssize_t recv(int sockfd, void buf[.len], size_t len,
>                          int flags);
>         ssize_t recvfrom(int sockfd, void buf[restrict .len], size_t len,
>                          int flags,
>                          struct sockaddr *_Nullable restrict src_addr,
>                          socklen_t *_Nullable restrict addrlen);
>         ssize_t recvmsg(int sockfd, struct msghdr *msg, int flags);

OK

> > uint32_t " sig ");
> > .sp
>
> .sp is unnecessary here.

OK

> .fi is missing (it's the "closing" pair to .nf).

OK

> > .SH DESCRIPTION
>
> Use .PP instead of blank lines.

OK

> > The
> > .BR rseq ()
> > ABI accelerates specific user-space operations by registering a
> > per-thread data structure shared between kernel and user-space. This
>
> Use semantic newlines.  See man-pages(7):
>
>     Use semantic newlines
>         In  the source of a manual page, new sentences should be started 
> on new
>         lines, long sentences should be split into lines at clause 
> breaks (com‐
>         mas, semicolons, colons, and so on), and long clauses should  
> be  split
>         at  phrase  boundaries.   This convention, sometimes known as 
> "semantic
>         newlines", makes it easier to see the effect of  patches,  
> which  often
>         operate at the level of individual sentences, clauses, or phrases.

OK, done across the entire the manual page source.

[...]

> > .PP
> > Some examples of operations that can be accelerated or improved
> > by this ABI:
> > .IP \[bu] 2
>
> Use 3 instead of 2.  See man-pages:
>
>     Lists
>         There are different kinds of lists:
>
>         [...]
>
>         Bullet lists
>                Elements  are  preceded by bullet symbols (\(bu).  
> Anything that
>                doesn’t fit elsewhere is usually covered by this type of 
> list.
>
>         [...]
>
>         There should always be exactly 2 spaces between the list symbol 
> and the
>         elements.  This doesn’t apply to "tagged paragraphs", which use 
> the de‐
>         fault indentation rules.
>
>
> The rationale for that was that if you use 1 space, then the list 
> introducer can be confused with the list contents.  Two spaces makes the 
> difference more clear.

OK

> Also, we use \(bu instead of \[bu].  I'm not particularly worried by 
> using it, but I prefer being consistent at which one we use.

OK

[...]

> > .TP
> > .in +4n
>
> I guess you're looking for .RS/.RE.  It would wrap all the indented 
> stuff, replacing .in.

OK.


> > .I cpu_id_start
> > Always-updated value of the CPU number on which the registered thread is
> > running. Its value is guaranteed to always be a possible CPU number,
> > even when rseq is not registered. Its value should always be confirmed by
>
> rseq (and maybe other cases around too) should be formatted in italics, 
> since it's a variable name (.I).

"rseq" here does not refer to a variable name, but rather to the system 
call. Should it be formatted in italics ? I format it as ".BR" elsewhere 
in the man page.

I'll push a first update without the italics changes, please let me know 
if there are still issues left.

[...]

> > .in +4n
> > .I flags
> > Flags indicating the restart behavior of this structure. Can be a 
> > combination
> > of:
> > .IP \[bu]
>
> This list should be a tagged paragraph instead.  See man-pages(7):
>
>     Lists
>         There are different kinds of lists:
>
>         Tagged paragraphs
>                These  are used for a list of tags and their 
> descriptions.  When
>                the tags are constants (either macros or numbers)  they  
> are  in
>                bold.  Use the .TP macro.
>
>                An example is this "Tagged paragraphs" subsection is itself.
>
>         [...]
>
>         Bullet lists
>                Elements  are  preceded by bullet symbols (\(bu).  
> Anything that
>                doesn’t fit elsewhere is usually covered by this type of 
> list.
>
>         [...]

OK

[...]

> > .SH RETURN VALUE
> > A return value of 0 indicates success. On error, \-1 is returned, and
> > .I errno
> > is set appropriately.
> >
> > .SH ERRORS
> > .TP
> > .B EINVAL
> > Either
> > .I flags
> > contains an invalid value, or
> > .I rseq
> > contains an address which is not appropriately aligned, or
> > .I rseq_len
> > contains an incorrect size.
> > .TP
> > .B ENOSYS
> > The
> > .BR rseq ()
> > system call is not implemented by this kernel.
> > .TP
> > .B EFAULT
> > .I rseq
> > is an invalid address.
>
> Doesn't this result in a SEGV?  It's trying to access invalid memory.  
> We had some discussion about this in other syscalls, and concluded that 
> that's undefined behavior, and a crash is valid behavior (and probably a 
> good thing to do), right?  I'm just curious about the view from the 
> kernel point of view.

If the registered rseq pointer / size points to invalid memory on rseq 
registration, the rseq registration system call will return -1, 
errno=EFAULT. If at some point _after_ registration the mapping becomes 
invalid (e.g. unmapped without prior unregistration), then a SIGSEGV can 
be triggered.

I was not aware of this discussion a regarding returning EFAULT errno vs 
SIGSEGV. If this is becoming a consensus across system calls to segfault 
 rather than return EFAULT errno, I'm open to improve sys_rseq accordingly.

> > .TP
> > .B EBUSY
> > Restartable sequence is already registered for this thread.
> > .TP
> > .B EPERM
> > The
> > .I sig
> > argument on unregistration does not match the signature received
> > on registration.
> >
> > .SH VERSIONS
> > The
> > .BR rseq ()
> > system call was added in Linux 4.18.
> >
> > .SH CONFORMING TO
>
> We call that section STANDARDS now.

OK

I've done a first pass applying all the relevant changes. I've pushed 
the changes into the librseq master branch. I may have omitted 
bold/italic for some identifiers. If it's the case, please let me know.

Thanks!

Mathieu

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com