Message-ID: <20191003165301.ihrlrgcldgun7dld@yavin.dot.cyphar.com>
Date:   Fri, 4 Oct 2019 02:53:01 +1000
From:   Aleksa Sarai <cyphar@...har.com>
To:     Jonathan Corbet <corbet@....net>
Cc:     Christian Brauner <christian.brauner@...ntu.com>,
        linux-kernel@...r.kernel.org, libc-alpha@...rceware.org,
        Christian Brauner <christian@...uner.io>,
        Federico Vaga <federico.vaga@...a.pv.it>,
        Alessia Mantegazza <amantegazza@...a.pv.it>,
        Guillaume Dore <corwin@...ssif.eu>, linux-doc@...r.kernel.org,
        linux-abi@...r.kernel.org, Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [PATCH] Documentation: update about adding syscalls

On 2019-10-03, Jonathan Corbet <corbet@....net> wrote:
> [Expanding CC a bit; this is the sort of change I'm reluctant to take
> without being sure it reflects what the community thinks.]
> 
> On Wed,  2 Oct 2019 17:14:37 +0200
> Christian Brauner <christian.brauner@...ntu.com> wrote:
> 
> > Add additional information on how to ensure that syscalls with structure
> > arguments are extensible and add a section about naming conventions to
> > follow when adding revised versions of already existing syscalls.
> > 
> > Co-Developed-by: Aleksa Sarai <cyphar@...har.com>
> > Signed-off-by: Aleksa Sarai <cyphar@...har.com>
> > Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
> > ---
> >  Documentation/process/adding-syscalls.rst | 82 +++++++++++++++++++----
> >  1 file changed, 70 insertions(+), 12 deletions(-)
> > 
> > diff --git a/Documentation/process/adding-syscalls.rst b/Documentation/process/adding-syscalls.rst
> > index 1c3a840d06b9..93e0221fbb9a 100644
> > --- a/Documentation/process/adding-syscalls.rst
> > +++ b/Documentation/process/adding-syscalls.rst
> > @@ -79,7 +79,7 @@ flags, and reject the system call (with ``EINVAL``) if it does::
> >  For more sophisticated system calls that involve a larger number of arguments,
> >  it's preferred to encapsulate the majority of the arguments into a structure
> >  that is passed in by pointer.  Such a structure can cope with future extension
> > -by including a size argument in the structure::
> > +by either including a size argument in the structure::
> >  
> >      struct xyzzy_params {
> >          u32 size; /* userspace sets p->size = sizeof(struct xyzzy_params) */
> > @@ -87,20 +87,56 @@ by including a size argument in the structure::
> >          u64 param_2;
> >          u64 param_3;
> >      };
> > +    int sys_xyzzy(struct xyzzy_params __user *uarg);
> > +    /* in case of -E2BIG, p->size is set to the in-kernel size and thus all
> > +       extensions after that offset are unsupported. */
> 
> That comment kind of threw me for a loop - this is the first mention of
> E2BIG and readers may not just know what's being talked about.  Especially
> since the comment suggests *not* actually returning an error.

I probably could've worded this better -- this comment describes what
userspace sees when they use the API (sched_setattr(2) is an example of
this style of API).

In the case where the kernel doesn't support a requested extension
(usize > ksize, and there are non-zero bytes past ksize) then the kernel
returns -E2BIG *but also* sets p->size to ksize so that userspace knows
what extensions the kernel supports.

Maybe I should've replicated more of the details from the kernel-doc for
copy_struct_from_user().

> > -As long as any subsequently added field, say ``param_4``, is designed so that a
> > -zero value gives the previous behaviour, then this allows both directions of
> > -version mismatch:
> > +or by including a separate argument that specifies the size::
> >  
> > - - To cope with a later userspace program calling an older kernel, the kernel
> > -   code should check that any memory beyond the size of the structure that it
> > -   expects is zero (effectively checking that ``param_4 == 0``).
> > - - To cope with an older userspace program calling a newer kernel, the kernel
> > -   code can zero-extend a smaller instance of the structure (effectively
> > -   setting ``param_4 = 0``).
> > +    struct xyzzy_params {
> > +        u32 param_1;
> > +        u64 param_2;
> > +        u64 param_3;
> > +    };
> > +    /* userspace sets @usize = sizeof(struct xyzzy_params) */
> > +    int sys_xyzzy(struct xyzzy_params __user *uarg, size_t usize);
> > +    /* in case of -E2BIG, userspace has to attempt smaller @usize values
> > +       to figure out which extensions are unsupported. */
> 
> Here too.  But what I'm really wondering about now is: you're describing
> different behavior for what are essentially two cases of the same thing.
> Why should the kernel simply accept the smaller size if the size is
> embedded in the structure itself, but return an error and force user space
> to retry if it's a separate argument?
> 
> I guess maybe because in the latter case the kernel can't easily return
> the size it's actually using?  I think that should be explicit if so.

As above, the -E2BIG only happens if userspace is trying to use an
extension that the kernel doesn't support (usize > ksize, non-zero bytes
after ksize). The main difference between the two API styles is whether
or not userspace gets told what ksize is explicitly in the case of an
-E2BIG.

Maybe it would be less confusing to only mention one of ways of doing
it, but then we have to pick one (and while the newer syscalls [clone3
and openat2] use a separate argument, there are more syscalls which
embed it in the struct).

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists