From: Andy Lutomirski <luto@kernel.org> Date: 2020-11-16 23:57:57
Linux 5.10 contains this patch:
commit 2a36ab717e8fe678d98f81c14a0b124712719840
Author: Peter Oskolkov [off-list ref]
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
----- 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 [off-list ref]
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.
From: "Theodore Y. Ts'o" <tytso@mit.edu> Date: 2020-11-17 17:06:01
On Mon, Nov 16, 2020 at 03:57:40PM -0800, Andy Lutomirski wrote:
Linux 5.10 contains this patch:
commit 2a36ab717e8fe678d98f81c14a0b124712719840
Author: Peter Oskolkov [off-list ref]
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
Linux 5.10 contains this patch:
commit 2a36ab717e8fe678d98f81c14a0b124712719840
Author: Peter Oskolkov [off-list ref]
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.
On Tue, Nov 17, 2020 at 06:16:28PM +0100, Florian Weimer wrote:
* Andy Lutomirski:
quoted
Linux 5.10 contains this patch:
commit 2a36ab717e8fe678d98f81c14a0b124712719840
Author: Peter Oskolkov [off-list ref]
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
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 [off-list ref]
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 [off-list ref]
Reviewed-by: Stefan Metzmacher [off-list ref]
<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.
From: Peter Oskolkov <hidden> Date: 2020-11-17 18:59:04
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 [off-list ref] wrote:
* Segher Boessenkool:
quoted
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 [off-list ref]
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 [off-list ref]
Reviewed-by: Stefan Metzmacher [off-list ref]
<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.
----- 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 [off-list ref] wrote:
quoted
* Segher Boessenkool:
quoted
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 [off-list ref]
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 [off-list ref]
Reviewed-by: Stefan Metzmacher [off-list ref]
<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.
From: Peter Oskolkov <hidden> Date: 2020-11-17 19:32:43
On Tue, Nov 17, 2020 at 11:21 AM Mathieu Desnoyers
[off-list ref] wrote:
----- On Nov 17, 2020, at 1:58 PM, Peter Oskolkov posk@google.com wrote:
quoted
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
[...]
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.
On 2020-11-16, Andy Lutomirski [off-list ref] wrote:
Linux 5.10 contains this patch:
commit 2a36ab717e8fe678d98f81c14a0b124712719840
Author: Peter Oskolkov [off-list ref]
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/>