Is adding an argument to an existing syscall okay?

Is adding an argument to an existing syscall okay?

[Andy Lutomirski]

Linux 5.10 contains this patch:

commit 2a36ab717e8fe678d98f81c14a0b124712719840
Author: Peter Oskolkov <posk@google.com>
Date:   Wed Sep 23 16:36:16 2020 -0700

    rseq/membarrier: Add MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ

This adds an argument to an existing syscall.  Before the patch,
membarrier had 2 parameters; now it has 3.  Is this really okay?  At
least the patch is careful and ignores the third parameter unless a
previously unused flag bit is set.

--Andy

Re: Is adding an argument to an existing syscall okay?

[Mathieu Desnoyers]

----- On Nov 16, 2020, at 6:57 PM, Andy Lutomirski luto@kernel.org wrote:

> Linux 5.10 contains this patch:
> 
> commit 2a36ab717e8fe678d98f81c14a0b124712719840
> Author: Peter Oskolkov <posk@google.com>
> Date:   Wed Sep 23 16:36:16 2020 -0700
> 
>    rseq/membarrier: Add MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ
> 
> This adds an argument to an existing syscall.  Before the patch,
> membarrier had 2 parameters; now it has 3.  Is this really okay?  At
> least the patch is careful and ignores the third parameter unless a
> previously unused flag bit is set.

Hi Andy,

I wondered about exactly this on August 12 2020:

https://lore.kernel.org/r/1477195446.6156.1597261492255.JavaMail.zimbra@efficios.com

And then on August 25, after receiving no feedback, I told Peter to try this approach:

https://lore.kernel.org/r/1336467655.17779.1598374701401.JavaMail.zimbra@efficios.com

and nobody complained until now. As you note, the extra argument is only used when
previously unused flag bits are set.

So your question is very relevant, and I still look forward to receiving feedback
on this matter.

Thanks,

Mathieu

> 
> --Andy

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

Re: Is adding an argument to an existing syscall okay?

[Theodore Y. Ts'o]

On Mon, Nov 16, 2020 at 03:57:40PM -0800, Andy Lutomirski wrote:
> Linux 5.10 contains this patch:
> 
> commit 2a36ab717e8fe678d98f81c14a0b124712719840
> Author: Peter Oskolkov <posk@google.com>
> Date:   Wed Sep 23 16:36:16 2020 -0700
> 
>     rseq/membarrier: Add MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ
> 
> This adds an argument to an existing syscall.  Before the patch,
> membarrier had 2 parameters; now it has 3.  Is this really okay?  At
> least the patch is careful and ignores the third parameter unless a
> previously unused flag bit is set.

So I can't see a way in which this would be problematic.  I guess it
might mean that strace might not be able to properly display the extra
parameter if it doesn't know about the new flag, but that would also
be true if we used part of a padding field for a new structure element.

Flipping around the question, why would this *NOT* be okay?

  	      	   	     	     	   - Ted

Re: Is adding an argument to an existing syscall okay?

[Florian Weimer]

* Andy Lutomirski:

> Linux 5.10 contains this patch:
>
> commit 2a36ab717e8fe678d98f81c14a0b124712719840
> Author: Peter Oskolkov <posk@google.com>
> Date:   Wed Sep 23 16:36:16 2020 -0700
>
>     rseq/membarrier: Add MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ
>
> This adds an argument to an existing syscall.  Before the patch,
> membarrier had 2 parameters; now it has 3.  Is this really okay?  At
> least the patch is careful and ignores the third parameter unless a
> previously unused flag bit is set.

It's really iffy.  It's hard to break this in system call wrappers on
x86-64, where we just load %eax and call into the kernel.  But on
architectures which require argument shuffling, it will break.

If there were a system call wrapper in glibc (my patch was rejected
due to lack of documentation fo the semantics, so we got lucky there),
we'd have to add a new symbol version for this.  It happened before in
the dark ages, repeatedly, but it's a bit disappointing to be in this
situation again.

In general the main problem I see is the poor source code
compatibility.  We really, really don't want variadic system call
wrappers, and we specifically do not want to introduce them
retroactively.  (Changing an implementation from non-variadic to
variadic is not an ABI-safe change on POWER and probably other
targets.)  So we'd require that from now on, the programmer has to
pass the zero argument explicitly.  Porting is simpler than the recent
futex_time64 breakage, but the downside is that immediately impacts
all targets.

Cc: linux-toolchains for ABI impact.

Re: Is adding an argument to an existing syscall okay?

[Segher Boessenkool]

On Tue, Nov 17, 2020 at 06:16:28PM +0100, Florian Weimer wrote:
> * Andy Lutomirski:
> 
> > Linux 5.10 contains this patch:
> >
> > commit 2a36ab717e8fe678d98f81c14a0b124712719840
> > Author: Peter Oskolkov <posk@google.com>
> > Date:   Wed Sep 23 16:36:16 2020 -0700
> >
> >     rseq/membarrier: Add MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ
> >
> > This adds an argument to an existing syscall.  Before the patch,
> > membarrier had 2 parameters; now it has 3.  Is this really okay?  At
> > least the patch is careful and ignores the third parameter unless a
> > previously unused flag bit is set.
> 
> It's really iffy.  It's hard to break this in system call wrappers on
> x86-64, where we just load %eax and call into the kernel.  But on
> architectures which require argument shuffling, it will break.
> 
> If there were a system call wrapper in glibc (my patch was rejected
> due to lack of documentation fo the semantics, so we got lucky there),
> we'd have to add a new symbol version for this.  It happened before in
> the dark ages, repeatedly, but it's a bit disappointing to be in this
> situation again.
> 
> In general the main problem I see is the poor source code
> compatibility.  We really, really don't want variadic system call
> wrappers, and we specifically do not want to introduce them
> retroactively.  (Changing an implementation from non-variadic to
> variadic is not an ABI-safe change on POWER and probably other
> targets.)

But this isn't variadic in the sense of "..." -- on Power that always
passes the unspecified arguments in memory, while in this case it just
passes in either two or three registers.  I don't know any arg where
that would not work, given the Linux system call restrictions.

This is similar to the "open" system call.

> So we'd require that from now on, the programmer has to
> pass the zero argument explicitly.  Porting is simpler than the recent
> futex_time64 breakage, but the downside is that immediately impacts
> all targets.
> 
> Cc: linux-toolchains for ABI impact.

It certainly would simplify matters if this was simply not done ;-)


Segher

Re: Is adding an argument to an existing syscall okay?

[Florian Weimer]

* Segher Boessenkool:

> But this isn't variadic in the sense of "..." -- on Power that always
> passes the unspecified arguments in memory, while in this case it just
> passes in either two or three registers.  I don't know any arg where
> that would not work, given the Linux system call restrictions.
>
> This is similar to the "open" system call.

Exactly.  You cannot call the open function through a non-variadic
function pointer.  I've seen it cause stack corruption in practice:

commit c7774174beffe9a8d29dd4fb38bbed43ece1cecd
Author: Andreas Schneider <asn@samba.org>
Date:   Wed Aug 2 13:21:59 2017 +0200

    swrap: Fix prototype of open[64] to prevent segfault on ppc64le
    
    The calling conventions for vaarg are different on ppc64le. The patch
    fixes segfaults on that platform.
    
    Thanks to Florian Weimer who helped debugging it!
    
    Signed-off-by: Andreas Schneider <asn@samba.org>
    Reviewed-by: Stefan Metzmacher <metze@samba.org>

<https://git.samba.org/?p=socket_wrapper.git;a=commitdiff;h=c7774174beffe>

It is possible to implement the open function in such a way that it
does not have this problem (simply do not use the parameter save area,
using assembler if necessary), but it's another obscure step that libc
implementers would have to take.

Re: Is adding an argument to an existing syscall okay?

[Peter Oskolkov]

My assumption here was that applications that are aware of the new API
will always provide three parameters, while older applications will
continue calling the syscall with two.

I can't think of a situation/architecture where this will break anything.

Thanks,
Peter


On Tue, Nov 17, 2020 at 10:44 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Segher Boessenkool:
>
> > But this isn't variadic in the sense of "..." -- on Power that always
> > passes the unspecified arguments in memory, while in this case it just
> > passes in either two or three registers.  I don't know any arg where
> > that would not work, given the Linux system call restrictions.
> >
> > This is similar to the "open" system call.
>
> Exactly.  You cannot call the open function through a non-variadic
> function pointer.  I've seen it cause stack corruption in practice:
>
> commit c7774174beffe9a8d29dd4fb38bbed43ece1cecd
> Author: Andreas Schneider <asn@samba.org>
> Date:   Wed Aug 2 13:21:59 2017 +0200
>
>     swrap: Fix prototype of open[64] to prevent segfault on ppc64le
>
>     The calling conventions for vaarg are different on ppc64le. The patch
>     fixes segfaults on that platform.
>
>     Thanks to Florian Weimer who helped debugging it!
>
>     Signed-off-by: Andreas Schneider <asn@samba.org>
>     Reviewed-by: Stefan Metzmacher <metze@samba.org>
>
> <https://git.samba.org/?p=socket_wrapper.git;a=commitdiff;h=c7774174beffe>
>
> It is possible to implement the open function in such a way that it
> does not have this problem (simply do not use the parameter save area,
> using assembler if necessary), but it's another obscure step that libc
> implementers would have to take.

Re: Is adding an argument to an existing syscall okay?

[Mathieu Desnoyers]

----- On Nov 17, 2020, at 1:58 PM, Peter Oskolkov posk@google.com wrote:

> My assumption here was that applications that are aware of the new API
> will always provide three parameters, while older applications will
> continue calling the syscall with two.
> 
> I can't think of a situation/architecture where this will break anything.

I think what Florian refers to here is if there would be a glibc library
wrapper exposing the system call to applications. There, the number of
arguments would matter. But it does not exist today.

In some sense, it's a good thing that there isn't such wrapper exposed
yet. It also makes me wonder whether exposing system calls directly as a
library ABI is a good thing. It appears that library ABIs have stronger
restrictions with respect to number and types of parameters than system
calls.

Thanks,

Mathieu

> 
> Thanks,
> Peter
> 
> 
> On Tue, Nov 17, 2020 at 10:44 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>>
>> * Segher Boessenkool:
>>
>> > But this isn't variadic in the sense of "..." -- on Power that always
>> > passes the unspecified arguments in memory, while in this case it just
>> > passes in either two or three registers.  I don't know any arg where
>> > that would not work, given the Linux system call restrictions.
>> >
>> > This is similar to the "open" system call.
>>
>> Exactly.  You cannot call the open function through a non-variadic
>> function pointer.  I've seen it cause stack corruption in practice:
>>
>> commit c7774174beffe9a8d29dd4fb38bbed43ece1cecd
>> Author: Andreas Schneider <asn@samba.org>
>> Date:   Wed Aug 2 13:21:59 2017 +0200
>>
>>     swrap: Fix prototype of open[64] to prevent segfault on ppc64le
>>
>>     The calling conventions for vaarg are different on ppc64le. The patch
>>     fixes segfaults on that platform.
>>
>>     Thanks to Florian Weimer who helped debugging it!
>>
>>     Signed-off-by: Andreas Schneider <asn@samba.org>
>>     Reviewed-by: Stefan Metzmacher <metze@samba.org>
>>
>> <https://git.samba.org/?p=socket_wrapper.git;a=commitdiff;h=c7774174beffe>
>>
>> It is possible to implement the open function in such a way that it
>> does not have this problem (simply do not use the parameter save area,
>> using assembler if necessary), but it's another obscure step that libc
> > implementers would have to take.

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

Re: Is adding an argument to an existing syscall okay?

[Peter Oskolkov]

On Tue, Nov 17, 2020 at 11:21 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Nov 17, 2020, at 1:58 PM, Peter Oskolkov posk@google.com wrote:
>
> > My assumption here was that applications that are aware of the new API
> > will always provide three parameters, while older applications will
> > continue calling the syscall with two.
> >
> > I can't think of a situation/architecture where this will break anything.
>
> I think what Florian refers to here is if there would be a glibc library
> wrapper exposing the system call to applications. There, the number of
> arguments would matter. But it does not exist today.
>
> In some sense, it's a good thing that there isn't such wrapper exposed
> yet. It also makes me wonder whether exposing system calls directly as a
> library ABI is a good thing. It appears that library ABIs have stronger
> restrictions with respect to number and types of parameters than system
> calls.

Technically, a library that exposes membarrier() with two parameters
can just add membarrier_ex() or whatever with three parameters, not
breaking anything. From the final user perspective, this would look exactly
as if we added a new syscall membarrier_ex.

So the question becomes whether it is better to add a new syscall and a new
library function, or just add another parameter to the existing
syscall, and a new
library function, and the answer to this question is more about policy
than about
technical merits of the two approaches.

I have no comments on policy matters here - this is up to maintainers. :)

Thanks,
Peter

[...]

Re: Is adding an argument to an existing syscall okay?

[Florian Weimer]

* Mathieu Desnoyers:

> In some sense, it's a good thing that there isn't such wrapper exposed
> yet. It also makes me wonder whether exposing system calls directly as a
> library ABI is a good thing. It appears that library ABIs have stronger
> restrictions with respect to number and types of parameters than system
> calls.

The generic syscall wrapper function cannot be used to call all system
calls on several architectures (e.g., if the kernel ABI has long as 64
bits, userspace has 32 bits, and the syscall function is *required* to
return a long value, not long long).  It may also be difficult to get
the argument promotion correctly.  _time64-only architectures have
source code impact even if time is not actually used in the call.  And
so on.

A wrapper-less approach is possible, I think, even with C.  But I do
not think that userspace source code portability without wrappers has
been a concern for system call design so far.  It's a very constrained
space already, so I'm not sure if it's a good idea to add further
rules.

Re: Is adding an argument to an existing syscall okay?

[Aleksa Sarai]

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

On 2020-11-16, Andy Lutomirski <luto@kernel.org> wrote:
> Linux 5.10 contains this patch:
> 
> commit 2a36ab717e8fe678d98f81c14a0b124712719840
> Author: Peter Oskolkov <posk@google.com>
> Date:   Wed Sep 23 16:36:16 2020 -0700
> 
>     rseq/membarrier: Add MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ
> 
> This adds an argument to an existing syscall.  Before the patch,
> membarrier had 2 parameters; now it has 3.  Is this really okay?  At
> least the patch is careful and ignores the third parameter unless a
> previously unused flag bit is set.

This change is not wrong from a correctness perspective but this kind of
pattern is (in my view at least) an anti-pattern -- as Florian has
mentioned, adding new arguments to an existing syscall can lead to libc
complications for wrappers, as well as more explicit issues like the
O_TMPFILE example.

This all harkens back to the whole "what extensibility system we want to
have" question Christian and I discussed at LPC this year[1]. This setup
(add a new flag to enable the new argument) is one of the approaches we
listed, but the only question is whether it's one we want to encourage.

In fairness, we can't retro-fit extensible structs to membarrier(2) so
there isn't a nice way to do this other than adding membarrier2(2). And
it seems to me the real issue is that membarrier(2) is a multiplexer
which inevitably means it has its own host of extensibility issues
(which we covered in the talk as well). *shrug*

[1]: https://lwn.net/Articles/830666/

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

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