Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 4 Oct 2019 02:53:01 +1000
From:   Aleksa Sarai <cyphar@cyphar.com>
To:     Jonathan Corbet <corbet@lwn.net>
Cc:     Christian Brauner <christian.brauner@ubuntu.com>,
        linux-kernel@vger.kernel.org, libc-alpha@sourceware.org,
        Christian Brauner <christian@brauner.io>,
        Federico Vaga <federico.vaga@vaga.pv.it>,
        Alessia Mantegazza <amantegazza@vaga.pv.it>,
        Guillaume Dore <corwin@poussif.eu>, linux-doc@vger.kernel.org,
        linux-abi@vger.kernel.org, Michael Kerrisk <mtk.manpages@gmail.com>
Subject: Re: [PATCH] Documentation: update about adding syscalls
Message-ID: <20191003165301.ihrlrgcldgun7dld@yavin.dot.cyphar.com>
References: <20191002151437.5367-1-christian.brauner@ubuntu.com>
 <20191003103931.52683cb6@lwn.net>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="ecmiy52y44gvxiaw"
Content-Disposition: inline
In-Reply-To: <20191003103931.52683cb6@lwn.net>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--ecmiy52y44gvxiaw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 2019-10-03, Jonathan Corbet <corbet@lwn.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.]
>=20
> On Wed,  2 Oct 2019 17:14:37 +0200
> Christian Brauner <christian.brauner@ubuntu.com> wrote:
>=20
> > 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.
> >=20
> > Co-Developed-by: Aleksa Sarai <cyphar@cyphar.com>
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> >  Documentation/process/adding-syscalls.rst | 82 +++++++++++++++++++----
> >  1 file changed, 70 insertions(+), 12 deletions(-)
> >=20
> > 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``) i=
f it does::
> >  For more sophisticated system calls that involve a larger number of ar=
guments,
> >  it's preferred to encapsulate the majority of the arguments into a str=
ucture
> >  that is passed in by pointer.  Such a structure can cope with future e=
xtension
> > -by including a size argument in the structure::
> > +by either including a size argument in the structure::
> > =20
> >      struct xyzzy_params {
> >          u32 size; /* userspace sets p->size =3D sizeof(struct xyzzy_pa=
rams) */
> > @@ -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 thu=
s all
> > +       extensions after that offset are unsupported. */
>=20
> 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 directi=
ons of
> > -version mismatch:
> > +or by including a separate argument that specifies the size::
> > =20
> > - - 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 =3D=3D 0``).
> > - - To cope with an older userspace program calling a newer kernel, the=
 kernel
> > -   code can zero-extend a smaller instance of the structure (effective=
ly
> > -   setting ``param_4 =3D 0``).
> > +    struct xyzzy_params {
> > +        u32 param_1;
> > +        u64 param_2;
> > +        u64 param_3;
> > +    };
> > +    /* userspace sets @usize =3D 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 valu=
es
> > +       to figure out which extensions are unsupported. */
>=20
> 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?
>=20
> 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).

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

--ecmiy52y44gvxiaw
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQSxZm6dtfE8gxLLfYqdlLljIbnQEgUCXZYnaQAKCRCdlLljIbnQ
EhlVAQCqbspnt/NT+1gkI4JVdUZFAWs0Fw/uNiDdbdMPIaf3JAEA/PeY3tJKd9bc
O9UhoWmz9VranrMXxxyR6e0LVw7J3wo=
=BOSH
-----END PGP SIGNATURE-----

--ecmiy52y44gvxiaw--