Recent discussion led to a solution for extending struct rseq. This is
an implementation of the proposed solution.
Now is a good time to agree on this scheme before the release of glibc
2.32, just in case there are small details to fix on the user-space
side in order to allow extending struct rseq.
Thanks,
Mathieu
Mathieu Desnoyers (4):
selftests: rseq: Use fixed value as rseq_len parameter
rseq: Allow extending struct rseq
selftests: rseq: define __rseq_abi with extensible size
selftests: rseq: print rseq extensible size in basic test
include/linux/sched.h | 4 +++
include/uapi/linux/rseq.h | 42 ++++++++++++++++++++--
kernel/rseq.c | 44 +++++++++++++++++++----
tools/testing/selftests/rseq/basic_test.c | 15 ++++++++
tools/testing/selftests/rseq/rseq.c | 8 +++--
5 files changed, 101 insertions(+), 12 deletions(-)
--
2.17.1
The rseq registration and unregistration expect a fixed-size length
(32 bytes). In preparation to extend struct rseq, pass a fixed value
rather than the size of the rseq structure.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
include/uapi/linux/rseq.h | 5 +++++
tools/testing/selftests/rseq/rseq.c | 5 ++---
2 files changed, 7 insertions(+), 3 deletions(-)
Add a __rseq_abi.flags "RSEQ_TLS_FLAG_SIZE", which indicates support for
extending struct rseq. This adds two new fields to struct rseq:
user_size and kernel_size.
The user_size field allows the size of the __rseq_abi definition (which
can be overridden by symbol interposition either by a preloaded library
or by the application) to be handed over to the kernel at registration.
This registration can be performed by a library, e.g. glibc, which does
not know there is interposition taking place.
The kernel_size is populated by the kernel when the "RSEQ_TLS_FLAG_SIZE"
flag is set in __rseq_abi.flags to the minimum between user_size and
the offset of the "end" field of struct rseq as known by the kernel.
This allows user-space to query which fields are effectively populated
by the kernel.
A rseq_size field is added to the task struct to keep track of the
"kernel_size" effective for each thread.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
include/linux/sched.h | 4 ++++
include/uapi/linux/rseq.h | 37 ++++++++++++++++++++++++++++++++--
kernel/rseq.c | 42 +++++++++++++++++++++++++++++++++------
3 files changed, 75 insertions(+), 8 deletions(-)
Print whether extensible size feature is supported by the kernel
and __rseq_abi definition, along with the contents of the kernel_size
field if it is available.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
tools/testing/selftests/rseq/basic_test.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
From: Peter Oskolkov <hidden> Date: 2020-07-14 17:25:00
At Google, we actually extended struct rseq (I will post the patches
here once they are fully deployed and we have specific
benefits/improvements to report). We did this by adding several fields
below __u32 flags (the last field currently), and correspondingly
increasing rseq_len in rseq() syscall. If the kernel does not know of
this extension, it will return -EINVAL due to an unexpected rseq_len;
then the application can either fall-back to the standard/upstream
rseq, or bail. If the kernel does know of this extension, it accepts
it. If the application passes the old rseq_len (32), the kernel knows
that this is an old application and treats it as such.
I looked through the archives, but I did not find specifically why the
pretty standard approach described above is considered inferior to the
one taken in this patch (freeze rseq_len at 32, add additional length
fields to struct rseq). Can these be summarized?
Thanks,
Peter
On Mon, Jul 13, 2020 at 8:04 PM Mathieu Desnoyers
[off-list ref] wrote:
quoted hunk
Add a __rseq_abi.flags "RSEQ_TLS_FLAG_SIZE", which indicates support for
extending struct rseq. This adds two new fields to struct rseq:
user_size and kernel_size.
The user_size field allows the size of the __rseq_abi definition (which
can be overridden by symbol interposition either by a preloaded library
or by the application) to be handed over to the kernel at registration.
This registration can be performed by a library, e.g. glibc, which does
not know there is interposition taking place.
The kernel_size is populated by the kernel when the "RSEQ_TLS_FLAG_SIZE"
flag is set in __rseq_abi.flags to the minimum between user_size and
the offset of the "end" field of struct rseq as known by the kernel.
This allows user-space to query which fields are effectively populated
by the kernel.
A rseq_size field is added to the task struct to keep track of the
"kernel_size" effective for each thread.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
include/linux/sched.h | 4 ++++
include/uapi/linux/rseq.h | 37 ++++++++++++++++++++++++++++++++--
kernel/rseq.c | 42 +++++++++++++++++++++++++++++++++------
3 files changed, 75 insertions(+), 8 deletions(-)
----- On Jul 14, 2020, at 1:24 PM, Peter Oskolkov posk@posk.io wrote:
At Google, we actually extended struct rseq (I will post the patches
here once they are fully deployed and we have specific
benefits/improvements to report). We did this by adding several fields
below __u32 flags (the last field currently), and correspondingly
increasing rseq_len in rseq() syscall. If the kernel does not know of
this extension, it will return -EINVAL due to an unexpected rseq_len;
then the application can either fall-back to the standard/upstream
rseq, or bail. If the kernel does know of this extension, it accepts
it. If the application passes the old rseq_len (32), the kernel knows
that this is an old application and treats it as such.
I looked through the archives, but I did not find specifically why the
pretty standard approach described above is considered inferior to the
one taken in this patch (freeze rseq_len at 32, add additional length
fields to struct rseq). Can these be summarized?
I think you don't face the issues I'm facing with libc rseq integration
because you control the entire user-space software ecosystem at Google.
The main issue we face is that the library responsible for registering
rseq (either glibc 2.32+, an early-adopter librseq library, or the
application) may very well not be the same library defining the __rseq_abi
symbol used in the global symbol table. Interposition with ld preload or
by defining the __rseq_abi in the program's executable are good examples
of this kind of scenario, and those use-cases are supported.
So the size of the __rseq_abi structure may be larger than the struct
rseq known by glibc (and eventually smaller, if future glibc versions
extend their __rseq_abi size but is loaded with an older program/library
doing __rseq_abi interposition).
So we need some way to allow code defining the __rseq_abi to let the kernel
know how much room is available, without necessarily requiring the code
responsible for rseq registration to be aware of that extended layout.
This is the purpose of the __rseq_abi.flags RSEQ_FLAG_TLS_SIZE and field
__rseq_abi.user_size.
And we need some way to allow the kernel to let user-space rseq critical
sections (user code) know how much of those fields are actually populated
by the kernel. This is the purpose of __rseq_abi.flags RSEQ_FLAG_TLS_SIZE
with __rseq_abi.kernel_size.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
From: Peter Oskolkov <hidden> Date: 2020-07-14 18:34:00
On Tue, Jul 14, 2020 at 10:43 AM Mathieu Desnoyers
[off-list ref] wrote:
----- On Jul 14, 2020, at 1:24 PM, Peter Oskolkov posk@posk.io wrote:
quoted
At Google, we actually extended struct rseq (I will post the patches
here once they are fully deployed and we have specific
benefits/improvements to report). We did this by adding several fields
below __u32 flags (the last field currently), and correspondingly
increasing rseq_len in rseq() syscall. If the kernel does not know of
this extension, it will return -EINVAL due to an unexpected rseq_len;
then the application can either fall-back to the standard/upstream
rseq, or bail. If the kernel does know of this extension, it accepts
it. If the application passes the old rseq_len (32), the kernel knows
that this is an old application and treats it as such.
I looked through the archives, but I did not find specifically why the
pretty standard approach described above is considered inferior to the
one taken in this patch (freeze rseq_len at 32, add additional length
fields to struct rseq). Can these be summarized?
I think you don't face the issues I'm facing with libc rseq integration
because you control the entire user-space software ecosystem at Google.
The main issue we face is that the library responsible for registering
rseq (either glibc 2.32+, an early-adopter librseq library, or the
application) may very well not be the same library defining the __rseq_abi
symbol used in the global symbol table. Interposition with ld preload or
by defining the __rseq_abi in the program's executable are good examples
of this kind of scenario, and those use-cases are supported.
So the size of the __rseq_abi structure may be larger than the struct
rseq known by glibc (and eventually smaller, if future glibc versions
extend their __rseq_abi size but is loaded with an older program/library
doing __rseq_abi interposition).
So we need some way to allow code defining the __rseq_abi to let the kernel
know how much room is available, without necessarily requiring the code
responsible for rseq registration to be aware of that extended layout.
This is the purpose of the __rseq_abi.flags RSEQ_FLAG_TLS_SIZE and field
__rseq_abi.user_size.
And we need some way to allow the kernel to let user-space rseq critical
sections (user code) know how much of those fields are actually populated
by the kernel. This is the purpose of __rseq_abi.flags RSEQ_FLAG_TLS_SIZE
with __rseq_abi.kernel_size.
Thanks, Mathieu, for the explanation. Yes, multiple unrelated
libraries having to share struct rseq complicates matters. Your
approach appears to be a way to reconcile the issues you outlined
above.
Thanks,
Peter
From: Carlos O'Donell <hidden> Date: 2020-07-14 20:55:21
On 7/13/20 11:03 PM, Mathieu Desnoyers wrote:
Recent discussion led to a solution for extending struct rseq. This is
an implementation of the proposed solution.
Now is a good time to agree on this scheme before the release of glibc
2.32, just in case there are small details to fix on the user-space
side in order to allow extending struct rseq.
Adding extensibility to the rseq registration process would be great,
but we are out of time for the glibc 2.32 release.
Should we revert rseq for glibc 2.32 and spend quality time discussing
the implications of an extensible design, something that Google already
says they are doing?
We can, with a clear head, and an agreed upon extension mechanism
include rseq in glibc 2.33 (release scheduled for Feburary 1st 2021).
We release time boxed every 6 months, no deviation, so you know when
your next merge window will be.
We have already done the hard work of fixing the nesting signal
handler issues, and glibc integration. If we revert today that will
also give time for Firefox and Chrome to adjust their sandboxes.
Do you wish to go forward with rseq as we have it in glibc 2.32,
or do you wish to revert rseq from glibc 2.32, discuss the extension
mechanism, and put it back into glibc 2.33 with adjustments?
--
Cheers,
Carlos.
From: Chris Kennelly <hidden> Date: 2020-07-15 02:34:54
On Tue, Jul 14, 2020 at 2:33 PM Peter Oskolkov [off-list ref] wrote:
On Tue, Jul 14, 2020 at 10:43 AM Mathieu Desnoyers
[off-list ref] wrote:
quoted
----- On Jul 14, 2020, at 1:24 PM, Peter Oskolkov posk@posk.io wrote:
quoted
At Google, we actually extended struct rseq (I will post the patches
here once they are fully deployed and we have specific
benefits/improvements to report). We did this by adding several fields
below __u32 flags (the last field currently), and correspondingly
increasing rseq_len in rseq() syscall. If the kernel does not know of
this extension, it will return -EINVAL due to an unexpected rseq_len;
then the application can either fall-back to the standard/upstream
rseq, or bail. If the kernel does know of this extension, it accepts
it. If the application passes the old rseq_len (32), the kernel knows
that this is an old application and treats it as such.
I looked through the archives, but I did not find specifically why the
pretty standard approach described above is considered inferior to the
one taken in this patch (freeze rseq_len at 32, add additional length
fields to struct rseq). Can these be summarized?
I think you don't face the issues I'm facing with libc rseq integration
because you control the entire user-space software ecosystem at Google.
The main issue we face is that the library responsible for registering
rseq (either glibc 2.32+, an early-adopter librseq library, or the
application) may very well not be the same library defining the __rseq_abi
symbol used in the global symbol table. Interposition with ld preload or
by defining the __rseq_abi in the program's executable are good examples
of this kind of scenario, and those use-cases are supported.
Does this work if/when we run out of bytes in the current sizeof(__rseq_abi)?
Which library provides the TLS symbol (and N bytes of storage) seems
sensitive to the choices the linker makes for us, once the symbol
sizes diverge.
quoted
So the size of the __rseq_abi structure may be larger than the struct
rseq known by glibc (and eventually smaller, if future glibc versions
extend their __rseq_abi size but is loaded with an older program/library
doing __rseq_abi interposition).
When glibc provides registration, is the anticipated use case that a
library would unregister and reregister each thread to "upgrade" it to
the most modern version of interface it knows about provided by the
kernel?
quoted
So we need some way to allow code defining the __rseq_abi to let the kernel
know how much room is available, without necessarily requiring the code
responsible for rseq registration to be aware of that extended layout.
This is the purpose of the __rseq_abi.flags RSEQ_FLAG_TLS_SIZE and field
__rseq_abi.user_size.
And we need some way to allow the kernel to let user-space rseq critical
sections (user code) know how much of those fields are actually populated
by the kernel. This is the purpose of __rseq_abi.flags RSEQ_FLAG_TLS_SIZE
with __rseq_abi.kernel_size.
I authored the userspace component
(https://github.com/google/tcmalloc/commit/ad136d45f75a273b934446699cef8b278c34ec6e)
that consumes the extensions Peter mentions and found that minimizing
the performance impact of their potential absence was a bit of a
challenge.
There, I could assume an all-or-nothing registration of the new
feature--limited only by kernel availability for thread
homogeneity--but inconsistencies across early adopter libraries would
mean each thread would have to examine its own TLS to determine if a
feature were available.
Chris
From: Christian Brauner <hidden> Date: 2020-07-15 11:38:57
On Mon, Jul 13, 2020 at 11:03:46PM -0400, Mathieu Desnoyers wrote:
quoted hunk
Add a __rseq_abi.flags "RSEQ_TLS_FLAG_SIZE", which indicates support for
extending struct rseq. This adds two new fields to struct rseq:
user_size and kernel_size.
The user_size field allows the size of the __rseq_abi definition (which
can be overridden by symbol interposition either by a preloaded library
or by the application) to be handed over to the kernel at registration.
This registration can be performed by a library, e.g. glibc, which does
not know there is interposition taking place.
The kernel_size is populated by the kernel when the "RSEQ_TLS_FLAG_SIZE"
flag is set in __rseq_abi.flags to the minimum between user_size and
the offset of the "end" field of struct rseq as known by the kernel.
This allows user-space to query which fields are effectively populated
by the kernel.
A rseq_size field is added to the task struct to keep track of the
"kernel_size" effective for each thread.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
include/linux/sched.h | 4 ++++
include/uapi/linux/rseq.h | 37 ++++++++++++++++++++++++++++++++--
kernel/rseq.c | 42 +++++++++++++++++++++++++++++++++------
3 files changed, 75 insertions(+), 8 deletions(-)
(Btw, this what I suggested - minus the user_size part - when I said
"expose the size of struct rseq" the kernel knows about. The approach
here is of course more general.)
It's pretty uncommon to use __u16 for sizes at least in public facing
structs. I'd suggest to use __u32 user_size and __u32 kernel_size and if
needed, insert padding. Seems you have done this in your union above
already.
+
+ /*
+ * Very last field of the structure, to calculate size excluding padding
+ * with offsetof().
+ */
+ char end[];
Hm, could this mess with alignment or break making the struct
extensible? Feels like you're adding new members always before this
which is also pretty non-standard in terms of how we'd usually extend
structs.
@@ -315,7 +322,7 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,/* Unregister rseq for current thread. */if(current->rseq!=rseq||!current->rseq)return-EINVAL;-if(rseq_len!=sizeof(*rseq))+if(rseq_len!=RSEQ_LEN_EXPECTED)
So I have to say that I think it's not a great to fix the length of the
rseq_len argument basically making it somewhat a nop. If I recall
correctly Florian said something about the rseq_len becoming part of the
glibc abi and that's why it can't be changed?
Is there any way we can avoid that so we can use the rseq_len argument
to have userspace pass down the size of struct rseq they know about?
It's really unintuitive to pass down an extensible struct but the length
argument associated with it is fixed.
I also think there should be some compile-time sanity checks here
similar to what we do in other places see e.g.
BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
So here should at least be sm like:
BUILD_BUG_ON(sizeof(struct rseq) != RSEQ_LEN_EXPECTED);
quoted hunk
return -EINVAL;
if (current->rseq_sig != sig)
return -EPERM;
From: Christian Brauner <hidden> Date: 2020-07-15 12:33:35
On Wed, Jul 15, 2020 at 01:38:51PM +0200, Christian Brauner wrote:
On Mon, Jul 13, 2020 at 11:03:46PM -0400, Mathieu Desnoyers wrote:
quoted
Add a __rseq_abi.flags "RSEQ_TLS_FLAG_SIZE", which indicates support for
extending struct rseq. This adds two new fields to struct rseq:
user_size and kernel_size.
The user_size field allows the size of the __rseq_abi definition (which
can be overridden by symbol interposition either by a preloaded library
or by the application) to be handed over to the kernel at registration.
This registration can be performed by a library, e.g. glibc, which does
not know there is interposition taking place.
The kernel_size is populated by the kernel when the "RSEQ_TLS_FLAG_SIZE"
flag is set in __rseq_abi.flags to the minimum between user_size and
the offset of the "end" field of struct rseq as known by the kernel.
This allows user-space to query which fields are effectively populated
by the kernel.
A rseq_size field is added to the task struct to keep track of the
"kernel_size" effective for each thread.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
include/linux/sched.h | 4 ++++
include/uapi/linux/rseq.h | 37 ++++++++++++++++++++++++++++++++--
kernel/rseq.c | 42 +++++++++++++++++++++++++++++++++------
3 files changed, 75 insertions(+), 8 deletions(-)
(Btw, this what I suggested - minus the user_size part - when I said
"expose the size of struct rseq" the kernel knows about. The approach
here is of course more general.)
It's pretty uncommon to use __u16 for sizes at least in public facing
structs. I'd suggest to use __u32 user_size and __u32 kernel_size and if
needed, insert padding. Seems you have done this in your union above
already.
quoted
+
+ /*
+ * Very last field of the structure, to calculate size excluding padding
+ * with offsetof().
+ */
+ char end[];
Hm, could this mess with alignment or break making the struct
extensible? Feels like you're adding new members always before this
which is also pretty non-standard in terms of how we'd usually extend
structs.
@@ -315,7 +322,7 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,/* Unregister rseq for current thread. */if(current->rseq!=rseq||!current->rseq)return-EINVAL;-if(rseq_len!=sizeof(*rseq))+if(rseq_len!=RSEQ_LEN_EXPECTED)
So I have to say that I think it's not a great to fix the length of the
rseq_len argument basically making it somewhat a nop. If I recall
correctly Florian said something about the rseq_len becoming part of the
glibc abi and that's why it can't be changed?
Is there any way we can avoid that so we can use the rseq_len argument
to have userspace pass down the size of struct rseq they know about?
It's really unintuitive to pass down an extensible struct but the length
argument associated with it is fixed.
I also think there should be some compile-time sanity checks here
similar to what we do in other places see e.g.
BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
So here should at least be sm like:
BUILD_BUG_ON(sizeof(struct rseq) != RSEQ_LEN_EXPECTED);
So here's a very free-wheeling draft of roughly what I had in mind. Not
even compile-tested just to illustrate what I'd change and sorry if that
code will make you sob in your hands:
From 2879e3c30dbe6ba0fc53884b1c41deaa444924a8 Mon Sep 17 00:00:00 2001
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: Mon, 13 Jul 2020 23:03:46 -0400
Subject: [PATCH] [UNTESTED] rseq: Allow extending struct rseq
Add a __rseq_abi.flags "RSEQ_TLS_FLAG_SIZE", which indicates support for
extending struct rseq. This adds two new fields to struct rseq:
user_size and kernel_size.
The user_size field allows the size of the __rseq_abi definition (which
can be overridden by symbol interposition either by a preloaded library
or by the application) to be handed over to the kernel at registration.
This registration can be performed by a library, e.g. glibc, which does
not know there is interposition taking place.
The kernel_size is populated by the kernel when the "RSEQ_TLS_FLAG_SIZE"
flag is set in __rseq_abi.flags to the minimum between user_size and
the offset of the "end" field of struct rseq as known by the kernel.
This allows user-space to query which fields are effectively populated
by the kernel.
A rseq_size field is added to the task struct to keep track of the
"kernel_size" effective for each thread.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Christian Brauner <redacted>
---
include/linux/sched.h | 4 +++
include/uapi/linux/rseq.h | 36 ++++++++++++++++++--
kernel/rseq.c | 72 ++++++++++++++++++++++++++++++++++++---
3 files changed, 105 insertions(+), 7 deletions(-)
@@ -349,10 +363,58 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,*ensuretheprovidedrseqisproperlyalignedandvalid.*/if(!IS_ALIGNED((unsignedlong)rseq,__alignof__(*rseq))||-rseq_len!=sizeof(*rseq))+rseq_len<RSEQ_SIZE_VER0)return-EINVAL;if(!access_ok(rseq,rseq_len))return-EFAULT;++/* Handle extensible struct rseq ABI. */+ret=get_user(tls_flags,&rseq->flags);+if(ret)+returnret;+if(tls_flags&RSEQ_TLS_FLAG_SIZE){+u32user_size,kernel_size,active_size;++/* Can probably be made nicer by using check_zeroed_user(). */+ret=get_user(user_size,&rseq->user_size);+if(ret)+returnret;+if(user_size!=0)+return-EINVAL;++ret=get_user(active_size,&rseq->active_size);+if(ret)+returnret;+if(active_size!=0)+return-EINVAL;++ret=get_user(active_size,&rseq->kernel_size);+if(ret)+returnret;+if(kernel_size!=0)+return-EINVAL;++/* Calculate the useable size. */+active_size=min_t(u32,rseq_len,RSEQ_SIZE_LATEST);+ret=put_user(active_size,&rseq->active_size);+if(ret)+returnret;++/* Let other users know what userspace used to register. */+ret=put_user(rseq_len,&rseq->user_size);+if(ret)+return-EFAULT;++/* Let other users know what size the kernel supports. */+ret=put_user(RSEQ_SIZE_LATEST,&rseq->kernel_size);+if(ret)+return-EFAULT;++current->rseq_size=active_size;+}else{+current->rseq_size=RSEQ_SIZE_VER0;+}+current->rseq=rseq;current->rseq_sig=sig;/*
----- On Jul 14, 2020, at 4:55 PM, carlos carlos@redhat.com wrote:
On 7/13/20 11:03 PM, Mathieu Desnoyers wrote:
quoted
Recent discussion led to a solution for extending struct rseq. This is
an implementation of the proposed solution.
Now is a good time to agree on this scheme before the release of glibc
2.32, just in case there are small details to fix on the user-space
side in order to allow extending struct rseq.
Adding extensibility to the rseq registration process would be great,
but we are out of time for the glibc 2.32 release.
Of course, and my goal is not to add this support for extensibility
before glibc 2.32, but merely to see if we need to change anything in
the way it uses rseq today (before the release) in order to facilitate
extensibility in the future.
Should we revert rseq for glibc 2.32 and spend quality time discussing
the implications of an extensible design, something that Google already
says they are doing?
Google's approach is limited to contexts simpler than multiple unrelated
libraries scenarios. Peter Oskolkov stated as a follow-up that my
extension approach would be one way to deal with problems associated
with sharing __rseq_abi between unrelated libraries:
https://lore.kernel.org/lkml/CAPNVh5fiCCJpyeLj_ciWzFrO4fasVXZNhpfKXJhJWJirXdJOjQ@mail.gmail.com/
The fact that Google already have their own rseq extensions internally
confirms that planning for extensibility is needed.
We can, with a clear head, and an agreed upon extension mechanism
include rseq in glibc 2.33 (release scheduled for Feburary 1st 2021).
We release time boxed every 6 months, no deviation, so you know when
your next merge window will be.
We have already done the hard work of fixing the nesting signal
handler issues, and glibc integration. If we revert today that will
also give time for Firefox and Chrome to adjust their sandboxes.
Do you wish to go forward with rseq as we have it in glibc 2.32,
or do you wish to revert rseq from glibc 2.32, discuss the extension
mechanism, and put it back into glibc 2.33 with adjustments?
So here we have a catch-22 situation. Linus wants to see how rseq
is being used before accepting additional features (ref.
https://lore.kernel.org/lkml/CAHk-=wjk-2c4XvWjdzc-bs9Hbgvy-p7ASSnKKphggr5qDoXRDQ@mail.gmail.com/).
This lack of ability to allow user-space to make any large-scale use
of the rseq system call in a coordinated fashion blocks wide use of rseq.
This coordination is supposed to be done by glibc, and I told
every user-space project maintainer who contacted me to hold off
using rseq until it is integrated into glibc. "tcmalloc" from Google
is the exception because they do not care about ABI compatibility with
other libraries (they are OK with a breakage and requiring upgrade).
The process I'm going through right now is checking what are our
options for extending rseq starting from the current ABI, just
to see if we are painting ourselves in a corner with the current
glibc integration. However, if we postpone integration of rseq
into glibc because of possible future extensibility features, those
may never happen because of the lack of usage feedback, due of lack
of users, due to lack of coordinated ABI registration.
At this point, the main question I would like answered is whether
it would be acceptable to increase the size and alignment of
the __rseq_abi symbol (which will be exposed by glibc) between
e.g. glibc 2.32 and 2.33. If it's not possible, then we can
find other solutions, for instance using an indirection with
a pointer to an extended structure, but this appears to be
slightly less efficient.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
----- On Jul 14, 2020, at 10:34 PM, Chris Kennelly ckennelly@google.com wrote:
On Tue, Jul 14, 2020 at 2:33 PM Peter Oskolkov [off-list ref] wrote:
quoted
On Tue, Jul 14, 2020 at 10:43 AM Mathieu Desnoyers
[off-list ref] wrote:
quoted
----- On Jul 14, 2020, at 1:24 PM, Peter Oskolkov posk@posk.io wrote:
quoted
At Google, we actually extended struct rseq (I will post the patches
here once they are fully deployed and we have specific
benefits/improvements to report). We did this by adding several fields
below __u32 flags (the last field currently), and correspondingly
increasing rseq_len in rseq() syscall. If the kernel does not know of
this extension, it will return -EINVAL due to an unexpected rseq_len;
then the application can either fall-back to the standard/upstream
rseq, or bail. If the kernel does know of this extension, it accepts
it. If the application passes the old rseq_len (32), the kernel knows
that this is an old application and treats it as such.
I looked through the archives, but I did not find specifically why the
pretty standard approach described above is considered inferior to the
one taken in this patch (freeze rseq_len at 32, add additional length
fields to struct rseq). Can these be summarized?
I think you don't face the issues I'm facing with libc rseq integration
because you control the entire user-space software ecosystem at Google.
The main issue we face is that the library responsible for registering
rseq (either glibc 2.32+, an early-adopter librseq library, or the
application) may very well not be the same library defining the __rseq_abi
symbol used in the global symbol table. Interposition with ld preload or
by defining the __rseq_abi in the program's executable are good examples
of this kind of scenario, and those use-cases are supported.
Does this work if/when we run out of bytes in the current sizeof(__rseq_abi)?
Only if all libraries/programs involved (including glibc) expect that the size
of the __rseq_abi can be the smallest possible subset, and only consider it
to be "extended" if specific information in the ABI tells them it is the case.
Which library provides the TLS symbol (and N bytes of storage) seems
sensitive to the choices the linker makes for us, once the symbol
sizes diverge.
AFAIU, a symbol defined in the main executable will have precedence over
a preloaded library, which has precedence over shared library dependencies,
e.g. glibc.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
----- On Jul 15, 2020, at 8:33 AM, Christian Brauner christian.brauner@ubuntu.com wrote:
[...]
So here's a very free-wheeling draft of roughly what I had in mind. Not
even compile-tested just to illustrate what I'd change and sorry if that
code will make you sob in your hands:
[...]
+ /*
+ * With __rseq_abi.flags RSEQ_TLS_FLAG_SIZE set, user_size should be
+ * statically initialized to offsetof(struct rseq, end).
+ */
+ __u32 user_size;
+ /*
+ * With __rseq_abi.flags RSEQ_TLS_FLAG_SIZE set, if the kernel supports
+ * extensible struct rseq ABI, the kernel_size field is populated by
+ * the kernel to the minimum between user_size and the offset of the
+ * "end" field within the struct rseq supported by the kernel on
+ * successful registration. Should be initialized to 0.
+ */
+ __u32 kernel_size;
Moving from __u16 to __u32 for both fields don't achieve much, and increase
the size of struct rseq (excluding padding) from 24 bytes to 28 bytes.
Note that the struct rseq alignment is 32 bytes. At 24 bytes, it leaves room
for exactly one 8 bytes pointer, which can be useful for future extensions.
If the size is increased to 28 bytes or more, then we're done and cannot
add a pointer.
+ __u32 active_size;
This additional field takes the very last bytes of padding we have in the
current layout.
} __attribute__((aligned(4 * sizeof(__u64))));
+#define RSEQ_SIZE_VER0 24 /* sizeof first published struct */
This is incorrect. The sizeof(struct_rseq) with its 32 bytes alignment is 32,
not 24. The padding at the end of the structure is considered as part of its
size, but we cannot rely on its content being zero-initialized based on the
C standard.
+#define RSEQ_SIZE_VER1 32 /* sizeof second published struct */
+#define RSEQ_SIZE_LATEST RSEQ_SIZE_VER1 /* sizeof last published struct */
+
rseq_len,
* ensure the provided rseq is properly aligned and valid.
*/
if (!IS_ALIGNED((unsigned long)rseq, __alignof__(*rseq)) ||
- rseq_len != sizeof(*rseq))
+ rseq_len < RSEQ_SIZE_VER0)
This could perhaps be changed for future kernels, but will break for existing
kernels as soon as rseq_len is increased. This is something we should have
planned for in the initial implementation of the system call, but here we are.
How do you envision that userspace would handle this failure from older kernels ?
Try again with a second system call passing RSEQ_SIZE_VER0 as argument ?
return -EINVAL;
if (!access_ok(rseq, rseq_len))
return -EFAULT;
+
+ /* Handle extensible struct rseq ABI. */
+ ret = get_user(tls_flags, &rseq->flags);
+ if (ret)
+ return ret;
+ if (tls_flags & RSEQ_TLS_FLAG_SIZE) {
+ u32 user_size, kernel_size, active_size;
+
+ /* Can probably be made nicer by using check_zeroed_user(). */
+ ret = get_user(user_size, &rseq->user_size);
+ if (ret)
+ return ret;
+ if (user_size != 0)
+ return -EINVAL;
+
+ ret = get_user(active_size, &rseq->active_size);
+ if (ret)
+ return ret;
+ if (active_size != 0)
+ return -EINVAL;
+
+ ret = get_user(active_size, &rseq->kernel_size);
I guess you mean kernel_size here.
+ if (ret)
+ return ret;
+ if (kernel_size != 0)
+ return -EINVAL;
+
+ /* Calculate the useable size. */
+ active_size = min_t(u32, rseq_len, RSEQ_SIZE_LATEST);
Where is the rseq_len supposed to come from in userspace ? Should it be
that the code doing the registration uses sizeof(struct rseq), or offsetof(struct rseq, end),
or should it read the content of __rseq_abi.user_size ?
+ ret = put_user(active_size, &rseq->active_size);
+ if (ret)
+ return ret;
+
+ /* Let other users know what userspace used to register. */
+ ret = put_user(rseq_len, &rseq->user_size);
+ if (ret)
+ return -EFAULT;
+
+ /* Let other users know what size the kernel supports. */
I am not sure what those 3 __u32 fields (user_size, kernel_size, and active_size),
plus use of the rseq_len syscall parameter, accomplish which was not accomplished
by my __u16 user_size + kernel_size approach ? If anything, it seems to make support
of older kernels which do not support an extended rseq_len parameter more complex.
Thanks,
Mathieu
From: Christian Brauner <hidden> Date: 2020-07-15 15:33:28
On Wed, Jul 15, 2020 at 11:10:47AM -0400, Mathieu Desnoyers wrote:
----- On Jul 15, 2020, at 8:33 AM, Christian Brauner christian.brauner@ubuntu.com wrote:
[...]
quoted
So here's a very free-wheeling draft of roughly what I had in mind. Not
even compile-tested just to illustrate what I'd change and sorry if that
code will make you sob in your hands:
[...]
quoted
+ /*
+ * With __rseq_abi.flags RSEQ_TLS_FLAG_SIZE set, user_size should be
+ * statically initialized to offsetof(struct rseq, end).
+ */
+ __u32 user_size;
+ /*
+ * With __rseq_abi.flags RSEQ_TLS_FLAG_SIZE set, if the kernel supports
+ * extensible struct rseq ABI, the kernel_size field is populated by
+ * the kernel to the minimum between user_size and the offset of the
+ * "end" field within the struct rseq supported by the kernel on
+ * successful registration. Should be initialized to 0.
+ */
+ __u32 kernel_size;
Moving from __u16 to __u32 for both fields don't achieve much, and increase
the size of struct rseq (excluding padding) from 24 bytes to 28 bytes.
Note that the struct rseq alignment is 32 bytes. At 24 bytes, it leaves room
for exactly one 8 bytes pointer, which can be useful for future extensions.
If the size is increased to 28 bytes or more, then we're done and cannot
add a pointer.
quoted
+ __u32 active_size;
This additional field takes the very last bytes of padding we have in the
current layout.
quoted
} __attribute__((aligned(4 * sizeof(__u64))));
+#define RSEQ_SIZE_VER0 24 /* sizeof first published struct */
This is incorrect. The sizeof(struct_rseq) with its 32 bytes alignment is 32,
not 24. The padding at the end of the structure is considered as part of its
size, but we cannot rely on its content being zero-initialized based on the
C standard.
quoted
+#define RSEQ_SIZE_VER1 32 /* sizeof second published struct */
+#define RSEQ_SIZE_LATEST RSEQ_SIZE_VER1 /* sizeof last published struct */
+
rseq_len,
* ensure the provided rseq is properly aligned and valid.
*/
if (!IS_ALIGNED((unsigned long)rseq, __alignof__(*rseq)) ||
- rseq_len != sizeof(*rseq))
+ rseq_len < RSEQ_SIZE_VER0)
This could perhaps be changed for future kernels, but will break for existing
kernels as soon as rseq_len is increased. This is something we should have
planned for in the initial implementation of the system call, but here we are.
How do you envision that userspace would handle this failure from older kernels ?
Try again with a second system call passing RSEQ_SIZE_VER0 as argument ?
quoted
return -EINVAL;
if (!access_ok(rseq, rseq_len))
return -EFAULT;
+
+ /* Handle extensible struct rseq ABI. */
+ ret = get_user(tls_flags, &rseq->flags);
+ if (ret)
+ return ret;
+ if (tls_flags & RSEQ_TLS_FLAG_SIZE) {
+ u32 user_size, kernel_size, active_size;
+
+ /* Can probably be made nicer by using check_zeroed_user(). */
+ ret = get_user(user_size, &rseq->user_size);
+ if (ret)
+ return ret;
+ if (user_size != 0)
+ return -EINVAL;
+
+ ret = get_user(active_size, &rseq->active_size);
+ if (ret)
+ return ret;
+ if (active_size != 0)
+ return -EINVAL;
+
+ ret = get_user(active_size, &rseq->kernel_size);
I guess you mean kernel_size here.
quoted
+ if (ret)
+ return ret;
+ if (kernel_size != 0)
+ return -EINVAL;
+
+ /* Calculate the useable size. */
+ active_size = min_t(u32, rseq_len, RSEQ_SIZE_LATEST);
Where is the rseq_len supposed to come from in userspace ? Should it be
that the code doing the registration uses sizeof(struct rseq), or offsetof(struct rseq, end),
or should it read the content of __rseq_abi.user_size ?
quoted
+ ret = put_user(active_size, &rseq->active_size);
+ if (ret)
+ return ret;
+
+ /* Let other users know what userspace used to register. */
+ ret = put_user(rseq_len, &rseq->user_size);
+ if (ret)
+ return -EFAULT;
+
+ /* Let other users know what size the kernel supports. */
I am not sure what those 3 __u32 fields (user_size, kernel_size, and active_size),
plus use of the rseq_len syscall parameter, accomplish which was not accomplished
by my __u16 user_size + kernel_size approach ? If anything, it seems to make support
of older kernels which do not support an extended rseq_len parameter more complex.
Yeah, fair point. I really just sketched this.
It seemed to me that what you might want to expose all three sizes in
some form, i.e. the size the kernel knows about, the size that
userspace knows about and the size in use. The advantage of exposing the
size the kernel itself knows about and the size that userspace knows
about is that you can infer the used size and you don't loose any
information. When you only register the kernel used size and the size
userspace knows about you can't necessarily infer what size the kernel
supports. But I suppose there's no obvious case where this is needed rn.
Thanks!
Christian
From: Carlos O'Donell <hidden> Date: 2020-07-16 13:39:45
On 7/15/20 9:02 AM, Mathieu Desnoyers wrote:
At this point, the main question I would like answered is whether
it would be acceptable to increase the size and alignment of
the __rseq_abi symbol (which will be exposed by glibc) between
e.g. glibc 2.32 and 2.33. If it's not possible, then we can
find other solutions, for instance using an indirection with
a pointer to an extended structure, but this appears to be
slightly less efficient.
The answer is always a soft "maybe" because it depends exactly
on how we do it and what consequences we are willing to accept
in the design.
For example, static applications that call dlopen will fail if
we increase the alignment beyond 32 because we had to special
case this scenario. Why did we have to special case it? Because
the "static" part of the runtime needs to create the initial
thread's static TLS space, and since it doesn't know apriori
what will be loaded in the shared library, it needs to make a
"best guess" at the alignment requirement at startup.
We need to discuss this and agree that it's OK. We already want
to deprecate dynamic loading from static applications, so this
may not be a problem in general, but I hope you see my point.
That there are corner cases to be considered and ironed out.
I want to see a detailed design document explaining the various
compatibility issues and how we solve them along with the way
the extension mechanism would work and how it would be compliant
with C/C++ language rules in userspace without adding undue burden
of potentially having to use atomic instructions all the time.
This includes discussing how the headers change. We should also
talk out the options for symbol versioning and their consequences.
I haven't seen enough details, and there isn't really enough
time to discuss this. I think it is *great* that we are discussing
it, but it's safest if we revert rseq, finish the discussion,
and then finalize the inclusion for 2.33 with these details
ironed out.
I feel like we've made all the technical process we need to actually
include rseq in glibc, but this discussion, and the google example
(even if it doesn't match our use case) shows that if we spend another
month hammering out the extension details could yield something we
can use for years to come while we work out other details e.g. cpu_opv.
I can set aside time in the next month to write up such a document
and discuss these issues with you and Florian. The text would form
even more of the language we'd have to include in the man page for
the feature.
In the meantime I think we should revert rseq in glibc and take
our time to hash this out without the looming deadline of August 1st
for the ABI going out the door.
I know this is disappointing, but I think in a month you'll look
back at this, we'll have Fedora Rawhide using the new extensible
version (and you'll be able to point people at that), and we'll
only be 5 months away from an official release with extensible
rseq.
Could you please respond to Florian's request to revert here?
https://sourceware.org/pipermail/libc-alpha/2020-July/116368.html
I'm looking for a Signed-off-by from you that you're OK with
reverting.
--
Cheers,
Carlos.
----- On Jul 16, 2020, at 9:39 AM, carlos carlos@redhat.com wrote:
On 7/15/20 9:02 AM, Mathieu Desnoyers wrote:
quoted
At this point, the main question I would like answered is whether
it would be acceptable to increase the size and alignment of
the __rseq_abi symbol (which will be exposed by glibc) between
e.g. glibc 2.32 and 2.33. If it's not possible, then we can
find other solutions, for instance using an indirection with
a pointer to an extended structure, but this appears to be
slightly less efficient.
The answer is always a soft "maybe" because it depends exactly
on how we do it and what consequences we are willing to accept
in the design.
For example, static applications that call dlopen will fail if
we increase the alignment beyond 32 because we had to special
case this scenario. Why did we have to special case it? Because
the "static" part of the runtime needs to create the initial
thread's static TLS space, and since it doesn't know apriori
what will be loaded in the shared library, it needs to make a
"best guess" at the alignment requirement at startup.
We need to discuss this and agree that it's OK. We already want
to deprecate dynamic loading from static applications, so this
may not be a problem in general, but I hope you see my point.
That there are corner cases to be considered and ironed out.
Note that I don't foresee we will explicitly need to increase
the alignment value for __rseq_abi beyond 32, but I was merely
asking this for sake of completeness, in case extending struct rseq
beyond a certain limit ever happens to increase the minimum
alignment.
I want to see a detailed design document explaining the various
compatibility issues and how we solve them along with the way
the extension mechanism would work and how it would be compliant
with C/C++ language rules in userspace without adding undue burden
of potentially having to use atomic instructions all the time.
This includes discussing how the headers change. We should also
talk out the options for symbol versioning and their consequences.
I haven't seen enough details, and there isn't really enough
time to discuss this. I think it is *great* that we are discussing
it, but it's safest if we revert rseq, finish the discussion,
and then finalize the inclusion for 2.33 with these details
ironed out.
Yes, absolutely.
I feel like we've made all the technical process we need to actually
include rseq in glibc, but this discussion, and the google example
(even if it doesn't match our use case) shows that if we spend another
month hammering out the extension details could yield something we
can use for years to come while we work out other details e.g. cpu_opv.
I can set aside time in the next month to write up such a document
and discuss these issues with you and Florian. The text would form
even more of the language we'd have to include in the man page for
the feature.
I'll do my best to secure some time to work with you on this in the
next month, but I will really have to focus on other projects which
I had to delay to make sure the rseq integration was ready for glibc
2.32.
In the meantime I think we should revert rseq in glibc and take
our time to hash this out without the looming deadline of August 1st
for the ABI going out the door.
I know this is disappointing, but I think in a month you'll look
back at this, we'll have Fedora Rawhide using the new extensible
version (and you'll be able to point people at that), and we'll
only be 5 months away from an official release with extensible
rseq.
If this delay gives us a future-proof extensible rseq ABI, I'm absolutely
for it!