The rseq rseq_cs.ptr.{ptr32,padding} uapi endianness handling is
entirely wrong on 32-bit little endian: a preprocessor logic mistake
wrongly uses the big endian field layout on 32-bit little endian
architectures.
Fortunately, those ptr32 accessors were never used within the kernel,
and only meant as a convenience for user-space.
Remove those and only leave the "ptr64" union field, as this is the only
thing really needed to express the ABI. Document how 32-bit
architectures are meant to interact with this "ptr64" union field.
Fixes: ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union, update includes")
Signed-off-by: Mathieu Desnoyers <[email protected]>
Cc: Florian Weimer <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Dave Watson <[email protected]>
Cc: Paul Turner <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Russell King <[email protected]>
Cc: "H . Peter Anvin" <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Ben Maurer <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Michael Kerrisk <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: Paul E. McKenney <[email protected]>
---
include/uapi/linux/rseq.h | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index 9a402fdb60e9..31290f2424a7 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -105,22 +105,13 @@ struct rseq {
* Read and set by the kernel. Set by user-space with single-copy
* atomicity semantics. This field should only be updated by the
* thread which registered this data structure. Aligned on 64-bit.
+ *
+ * 32-bit architectures should update the low order bits of the
+ * rseq_cs.ptr64 field, leaving the high order bits initialized
+ * to 0.
*/
union {
__u64 ptr64;
-#ifdef __LP64__
- __u64 ptr;
-#else
- struct {
-#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined(__BIG_ENDIAN)
- __u32 padding; /* Initialized to zero. */
- __u32 ptr32;
-#else /* LITTLE */
- __u32 ptr32;
- __u32 padding; /* Initialized to zero. */
-#endif /* ENDIAN */
- } ptr;
-#endif
} rseq_cs;
/*
--
2.17.1
On Mon, Jan 24, 2022 at 12:12:40PM -0500, Mathieu Desnoyers wrote:
> The rseq rseq_cs.ptr.{ptr32,padding} uapi endianness handling is
> entirely wrong on 32-bit little endian: a preprocessor logic mistake
> wrongly uses the big endian field layout on 32-bit little endian
> architectures.
>
> Fortunately, those ptr32 accessors were never used within the kernel,
> and only meant as a convenience for user-space.
>
> Remove those and only leave the "ptr64" union field, as this is the only
> thing really needed to express the ABI. Document how 32-bit
> architectures are meant to interact with this "ptr64" union field.
>
> Fixes: ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union, update includes")
> Signed-off-by: Mathieu Desnoyers <[email protected]>
> Cc: Florian Weimer <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> Cc: Peter Zijlstra <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Dave Watson <[email protected]>
> Cc: Paul Turner <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: "H . Peter Anvin" <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Ben Maurer <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Michael Kerrisk <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> Cc: Paul E. McKenney <[email protected]>
> ---
> include/uapi/linux/rseq.h | 17 ++++-------------
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> index 9a402fdb60e9..31290f2424a7 100644
> --- a/include/uapi/linux/rseq.h
> +++ b/include/uapi/linux/rseq.h
> @@ -105,22 +105,13 @@ struct rseq {
> * Read and set by the kernel. Set by user-space with single-copy
> * atomicity semantics. This field should only be updated by the
> * thread which registered this data structure. Aligned on 64-bit.
> + *
> + * 32-bit architectures should update the low order bits of the
> + * rseq_cs.ptr64 field, leaving the high order bits initialized
> + * to 0.
> */
> union {
A bit unfortunate we seem to have to keep the union around even though
it's just one field now.
----- On Jan 25, 2022, at 7:21 AM, Christian Brauner [email protected] wrote:
> On Mon, Jan 24, 2022 at 12:12:40PM -0500, Mathieu Desnoyers wrote:
>> The rseq rseq_cs.ptr.{ptr32,padding} uapi endianness handling is
>> entirely wrong on 32-bit little endian: a preprocessor logic mistake
>> wrongly uses the big endian field layout on 32-bit little endian
>> architectures.
>>
>> Fortunately, those ptr32 accessors were never used within the kernel,
>> and only meant as a convenience for user-space.
>>
>> Remove those and only leave the "ptr64" union field, as this is the only
>> thing really needed to express the ABI. Document how 32-bit
>> architectures are meant to interact with this "ptr64" union field.
>>
>> Fixes: ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union, update
>> includes")
>> Signed-off-by: Mathieu Desnoyers <[email protected]>
>> Cc: Florian Weimer <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: [email protected]
>> Cc: Peter Zijlstra <[email protected]>
>> Cc: Boqun Feng <[email protected]>
>> Cc: Andy Lutomirski <[email protected]>
>> Cc: Dave Watson <[email protected]>
>> Cc: Paul Turner <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Russell King <[email protected]>
>> Cc: "H . Peter Anvin" <[email protected]>
>> Cc: Andi Kleen <[email protected]>
>> Cc: Christian Brauner <[email protected]>
>> Cc: Ben Maurer <[email protected]>
>> Cc: Steven Rostedt <[email protected]>
>> Cc: Josh Triplett <[email protected]>
>> Cc: Linus Torvalds <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Michael Kerrisk <[email protected]>
>> Cc: Joel Fernandes <[email protected]>
>> Cc: Paul E. McKenney <[email protected]>
>> ---
>> include/uapi/linux/rseq.h | 17 ++++-------------
>> 1 file changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
>> index 9a402fdb60e9..31290f2424a7 100644
>> --- a/include/uapi/linux/rseq.h
>> +++ b/include/uapi/linux/rseq.h
>> @@ -105,22 +105,13 @@ struct rseq {
>> * Read and set by the kernel. Set by user-space with single-copy
>> * atomicity semantics. This field should only be updated by the
>> * thread which registered this data structure. Aligned on 64-bit.
>> + *
>> + * 32-bit architectures should update the low order bits of the
>> + * rseq_cs.ptr64 field, leaving the high order bits initialized
>> + * to 0.
>> */
>> union {
>
> A bit unfortunate we seem to have to keep the union around even though
> it's just one field now.
Well, as far as the user-space projects that I know of which use rseq
are concerned (glibc, librseq, tcmalloc), those end up with their own
copy of the uapi header anyway to deal with the big/little endian field
on 32-bit. So I'm very much open to remove the union if we accept that
this uapi header is really just meant to express the ABI and is not
expected to be used as an API by user-space.
That would mean we also bring a uapi header copy into the kernel
rseq selftests as well to minimize the gap between librseq and
the kernel sefltests (the kernel sefltests pretty much include a
copy of librseq for convenience. librseq is maintained out of tree).
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers [email protected] wrote:
> ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner [email protected] wrote:
[...]
>>> include/uapi/linux/rseq.h | 17 ++++-------------
[...]
>>> union {
>>
>> A bit unfortunate we seem to have to keep the union around even though
>> it's just one field now.
>
> Well, as far as the user-space projects that I know of which use rseq
> are concerned (glibc, librseq, tcmalloc), those end up with their own
> copy of the uapi header anyway to deal with the big/little endian field
> on 32-bit. So I'm very much open to remove the union if we accept that
> this uapi header is really just meant to express the ABI and is not
> expected to be used as an API by user-space.
>
> That would mean we also bring a uapi header copy into the kernel
> rseq selftests as well to minimize the gap between librseq and
> the kernel sefltests (the kernel sefltests pretty much include a
> copy of librseq for convenience. librseq is maintained out of tree).
>
> Thoughts ?
Actually, if we go ahead and remove the union, and replace:
struct rseq {
union {
__u64 ptr64;
} rseq_cs;
[...]
} v;
by:
struct rseq {
__u64 rseq_cs;
} v;
expressions such as these are unchanged:
- sizeof(v.rseq_cs),
- &v.rseq_cs,
- __alignof__(v.rseq_cs),
- offsetof(struct rseq, rseq_cs).
So users of the uapi rseq.h (as an API) can still use rseq_abi->rseq_cs before
and after the change.
Based on this, I am inclined to remove the union, and just make the rseq_cs field
a __u64.
Any objections ?
Thanks,
Mathieu
>
> Thanks,
>
> Mathieu
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Tue, Jan 25, 2022 at 02:00:48PM -0500, Mathieu Desnoyers wrote:
> ----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers [email protected] wrote:
>
> > ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner [email protected] wrote:
> [...]
> >>> include/uapi/linux/rseq.h | 17 ++++-------------
> [...]
> >>> union {
> >>
> >> A bit unfortunate we seem to have to keep the union around even though
> >> it's just one field now.
> >
> > Well, as far as the user-space projects that I know of which use rseq
> > are concerned (glibc, librseq, tcmalloc), those end up with their own
> > copy of the uapi header anyway to deal with the big/little endian field
> > on 32-bit. So I'm very much open to remove the union if we accept that
> > this uapi header is really just meant to express the ABI and is not
> > expected to be used as an API by user-space.
> >
> > That would mean we also bring a uapi header copy into the kernel
> > rseq selftests as well to minimize the gap between librseq and
> > the kernel sefltests (the kernel sefltests pretty much include a
> > copy of librseq for convenience. librseq is maintained out of tree).
> >
> > Thoughts ?
>
> Actually, if we go ahead and remove the union, and replace:
>
> struct rseq {
> union {
> __u64 ptr64;
> } rseq_cs;
> [...]
> } v;
>
> by:
>
> struct rseq {
> __u64 rseq_cs;
> } v;
>
> expressions such as these are unchanged:
>
> - sizeof(v.rseq_cs),
> - &v.rseq_cs,
> - __alignof__(v.rseq_cs),
> - offsetof(struct rseq, rseq_cs).
>
> So users of the uapi rseq.h (as an API) can still use rseq_abi->rseq_cs before
> and after the change.
>
> Based on this, I am inclined to remove the union, and just make the rseq_cs field
> a __u64.
>
> Any objections ?
I do like it fwiw. But since I haven't been heavily involved in the
userspace usage of this I can't speak confidently to the regression
potential of a change like this. But I would think that we should risk
it instead of dragging a pointless union around forever.
* Christian Brauner:
> On Tue, Jan 25, 2022 at 02:00:48PM -0500, Mathieu Desnoyers wrote:
>> So users of the uapi rseq.h (as an API) can still use
>> rseq_abi->rseq_cs before and after the change.
>>
>> Based on this, I am inclined to remove the union, and just make the
>> rseq_cs field a __u64.
>>
>> Any objections ?
>
> I do like it fwiw. But since I haven't been heavily involved in the
> userspace usage of this I can't speak confidently to the regression
> potential of a change like this. But I would think that we should risk
> it instead of dragging a pointless union around forever.
I don't think glibc needs changes for this, it will keep building just
fine. We'll need to adjust the included kernel header fragment that
could be used by applications in some corner cases, but that's it.
From: Mathieu Desnoyers
> Sent: 25 January 2022 19:01
>
> ----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers [email protected] wrote:
>
> > ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner [email protected] wrote:
> [...]
> >>> include/uapi/linux/rseq.h | 17 ++++-------------
> [...]
> >>> union {
> >>
> >> A bit unfortunate we seem to have to keep the union around even though
> >> it's just one field now.
> >
> > Well, as far as the user-space projects that I know of which use rseq
> > are concerned (glibc, librseq, tcmalloc), those end up with their own
> > copy of the uapi header anyway to deal with the big/little endian field
> > on 32-bit. So I'm very much open to remove the union if we accept that
> > this uapi header is really just meant to express the ABI and is not
> > expected to be used as an API by user-space.
> >
> > That would mean we also bring a uapi header copy into the kernel
> > rseq selftests as well to minimize the gap between librseq and
> > the kernel sefltests (the kernel sefltests pretty much include a
> > copy of librseq for convenience. librseq is maintained out of tree).
> >
> > Thoughts ?
>
> Actually, if we go ahead and remove the union, and replace:
>
> struct rseq {
> union {
> __u64 ptr64;
> } rseq_cs;
> [...]
> } v;
>
> by:
>
> struct rseq {
> __u64 rseq_cs;
> } v;
>
> expressions such as these are unchanged:
>
> - sizeof(v.rseq_cs),
> - &v.rseq_cs,
> - __alignof__(v.rseq_cs),
> - offsetof(struct rseq, rseq_cs).
>
> So users of the uapi rseq.h (as an API) can still use rseq_abi->rseq_cs before
> and after the change.
But:
v.rseq_cs.ptr_64 = (uintptr_t)&foo;
is broken.
> Based on this, I am inclined to remove the union, and just make the rseq_cs field
> a __u64.
It really is a shame that you can't do:
void *rseq_cs __attribute__((size(8)));
and have the compiler just DTRT on 32bit systems.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
----- On Jan 26, 2022, at 12:16 PM, David Laight [email protected] wrote:
> From: Mathieu Desnoyers
>> Sent: 25 January 2022 19:01
>>
>> ----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers
>> [email protected] wrote:
>>
>> > ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner [email protected] wrote:
>> [...]
>> >>> include/uapi/linux/rseq.h | 17 ++++-------------
>> [...]
>> >>> union {
>> >>
>> >> A bit unfortunate we seem to have to keep the union around even though
>> >> it's just one field now.
>> >
>> > Well, as far as the user-space projects that I know of which use rseq
>> > are concerned (glibc, librseq, tcmalloc), those end up with their own
>> > copy of the uapi header anyway to deal with the big/little endian field
>> > on 32-bit. So I'm very much open to remove the union if we accept that
>> > this uapi header is really just meant to express the ABI and is not
>> > expected to be used as an API by user-space.
>> >
>> > That would mean we also bring a uapi header copy into the kernel
>> > rseq selftests as well to minimize the gap between librseq and
>> > the kernel sefltests (the kernel sefltests pretty much include a
>> > copy of librseq for convenience. librseq is maintained out of tree).
>> >
>> > Thoughts ?
>>
>> Actually, if we go ahead and remove the union, and replace:
>>
>> struct rseq {
>> union {
>> __u64 ptr64;
>> } rseq_cs;
>> [...]
>> } v;
>>
>> by:
>>
>> struct rseq {
>> __u64 rseq_cs;
>> } v;
>>
>> expressions such as these are unchanged:
>>
>> - sizeof(v.rseq_cs),
>> - &v.rseq_cs,
>> - __alignof__(v.rseq_cs),
>> - offsetof(struct rseq, rseq_cs).
>>
>> So users of the uapi rseq.h (as an API) can still use rseq_abi->rseq_cs before
>> and after the change.
>
> But:
> v.rseq_cs.ptr_64 = (uintptr_t)&foo;
> is broken.
True. But v.rseq_cs.ptr (on 64-bit) and v.rseq_cs.ptr.ptr32 (on 32-bit) are also
broken with the planned field removal. So how is the v.rseq_cs_ptr64 situation
different ?
My thinking here is that it does not matter if we break compilation for some
users of the uapi as an API as long as the ABI stays the same, especially
considering that all known users implement their own copy of the header.
I suspect that as far as the API is concerned, it is nice that we have at least
one way to access the field which works both before and after the change.
Simply using "v.rseq_cs" works both before/after for all use-cases that seem
to matter here.
>
>> Based on this, I am inclined to remove the union, and just make the rseq_cs
>> field
>> a __u64.
>
> It really is a shame that you can't do:
> void *rseq_cs __attribute__((size(8)));
> and have the compiler just DTRT on 32bit systems.
Indeed, the "size" directive appears to be ignored by the compiler.
Thanks,
Mathieu
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
> UK
> Registration No: 1397386 (Wales)
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
The rseq rseq_cs.ptr.{ptr32,padding} uapi endianness handling is
entirely wrong on 32-bit little endian: a preprocessor logic mistake
wrongly uses the big endian field layout on 32-bit little endian
architectures.
Fortunately, those ptr32 accessors were never used within the kernel,
and only meant as a convenience for user-space.
Remove those and replace the whole rseq_cs union by a __u64 type, as
this is the only thing really needed to express the ABI. Document how
32-bit architectures are meant to interact with this field.
Fixes: ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union, update includes")
Signed-off-by: Mathieu Desnoyers <[email protected]>
Cc: Florian Weimer <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Dave Watson <[email protected]>
Cc: Paul Turner <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Russell King <[email protected]>
Cc: "H . Peter Anvin" <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Ben Maurer <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Michael Kerrisk <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: Paul E. McKenney <[email protected]>
---
include/uapi/linux/rseq.h | 20 ++++----------------
kernel/rseq.c | 8 ++++----
2 files changed, 8 insertions(+), 20 deletions(-)
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index 9a402fdb60e9..77ee207623a9 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -105,23 +105,11 @@ struct rseq {
* Read and set by the kernel. Set by user-space with single-copy
* atomicity semantics. This field should only be updated by the
* thread which registered this data structure. Aligned on 64-bit.
+ *
+ * 32-bit architectures should update the low order bits of the
+ * rseq_cs field, leaving the high order bits initialized to 0.
*/
- union {
- __u64 ptr64;
-#ifdef __LP64__
- __u64 ptr;
-#else
- struct {
-#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined(__BIG_ENDIAN)
- __u32 padding; /* Initialized to zero. */
- __u32 ptr32;
-#else /* LITTLE */
- __u32 ptr32;
- __u32 padding; /* Initialized to zero. */
-#endif /* ENDIAN */
- } ptr;
-#endif
- } rseq_cs;
+ __u64 rseq_cs;
/*
* Restartable sequences flags field.
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 6d45ac3dae7f..97ac20b4f738 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -128,10 +128,10 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
int ret;
#ifdef CONFIG_64BIT
- if (get_user(ptr, &t->rseq->rseq_cs.ptr64))
+ if (get_user(ptr, &t->rseq->rseq_cs))
return -EFAULT;
#else
- if (copy_from_user(&ptr, &t->rseq->rseq_cs.ptr64, sizeof(ptr)))
+ if (copy_from_user(&ptr, &t->rseq->rseq_cs, sizeof(ptr)))
return -EFAULT;
#endif
if (!ptr) {
@@ -217,9 +217,9 @@ static int clear_rseq_cs(struct task_struct *t)
* Set rseq_cs to NULL.
*/
#ifdef CONFIG_64BIT
- return put_user(0UL, &t->rseq->rseq_cs.ptr64);
+ return put_user(0UL, &t->rseq->rseq_cs);
#else
- if (clear_user(&t->rseq->rseq_cs.ptr64, sizeof(t->rseq->rseq_cs.ptr64)))
+ if (clear_user(&t->rseq->rseq_cs, sizeof(t->rseq->rseq_cs)))
return -EFAULT;
return 0;
#endif
--
2.17.1
On Thu, Jan 27, 2022 at 10:27:20AM -0500, Mathieu Desnoyers wrote:
> The rseq rseq_cs.ptr.{ptr32,padding} uapi endianness handling is
> entirely wrong on 32-bit little endian: a preprocessor logic mistake
> wrongly uses the big endian field layout on 32-bit little endian
> architectures.
>
> Fortunately, those ptr32 accessors were never used within the kernel,
> and only meant as a convenience for user-space.
>
> Remove those and replace the whole rseq_cs union by a __u64 type, as
> this is the only thing really needed to express the ABI. Document how
> 32-bit architectures are meant to interact with this field.
>
> Fixes: ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union, update includes")
> Signed-off-by: Mathieu Desnoyers <[email protected]>
> Cc: Florian Weimer <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> Cc: Peter Zijlstra <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Dave Watson <[email protected]>
> Cc: Paul Turner <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: "H . Peter Anvin" <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Ben Maurer <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Michael Kerrisk <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> Cc: Paul E. McKenney <[email protected]>
> ---
Looks way cleaner now! Fwiw,
Acked-by: Christian Brauner <[email protected]>
> include/uapi/linux/rseq.h | 20 ++++----------------
> kernel/rseq.c | 8 ++++----
> 2 files changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> index 9a402fdb60e9..77ee207623a9 100644
> --- a/include/uapi/linux/rseq.h
> +++ b/include/uapi/linux/rseq.h
> @@ -105,23 +105,11 @@ struct rseq {
> * Read and set by the kernel. Set by user-space with single-copy
> * atomicity semantics. This field should only be updated by the
> * thread which registered this data structure. Aligned on 64-bit.
> + *
> + * 32-bit architectures should update the low order bits of the
> + * rseq_cs field, leaving the high order bits initialized to 0.
> */
> - union {
> - __u64 ptr64;
> -#ifdef __LP64__
> - __u64 ptr;
> -#else
> - struct {
> -#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined(__BIG_ENDIAN)
> - __u32 padding; /* Initialized to zero. */
> - __u32 ptr32;
> -#else /* LITTLE */
> - __u32 ptr32;
> - __u32 padding; /* Initialized to zero. */
> -#endif /* ENDIAN */
> - } ptr;
> -#endif
> - } rseq_cs;
> + __u64 rseq_cs;
>
> /*
> * Restartable sequences flags field.
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 6d45ac3dae7f..97ac20b4f738 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -128,10 +128,10 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
> int ret;
>
> #ifdef CONFIG_64BIT
> - if (get_user(ptr, &t->rseq->rseq_cs.ptr64))
> + if (get_user(ptr, &t->rseq->rseq_cs))
> return -EFAULT;
> #else
> - if (copy_from_user(&ptr, &t->rseq->rseq_cs.ptr64, sizeof(ptr)))
> + if (copy_from_user(&ptr, &t->rseq->rseq_cs, sizeof(ptr)))
> return -EFAULT;
> #endif
> if (!ptr) {
> @@ -217,9 +217,9 @@ static int clear_rseq_cs(struct task_struct *t)
> * Set rseq_cs to NULL.
> */
> #ifdef CONFIG_64BIT
> - return put_user(0UL, &t->rseq->rseq_cs.ptr64);
> + return put_user(0UL, &t->rseq->rseq_cs);
> #else
> - if (clear_user(&t->rseq->rseq_cs.ptr64, sizeof(t->rseq->rseq_cs.ptr64)))
> + if (clear_user(&t->rseq->rseq_cs, sizeof(t->rseq->rseq_cs)))
> return -EFAULT;
> return 0;
> #endif
> --
> 2.17.1
>
The following commit has been merged into the sched/core branch of tip:
Commit-ID: bfdf4e6208051ed7165b2e92035b4bf11f43eb63
Gitweb: https://git.kernel.org/tip/bfdf4e6208051ed7165b2e92035b4bf11f43eb63
Author: Mathieu Desnoyers <[email protected]>
AuthorDate: Thu, 27 Jan 2022 10:27:20 -05:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Wed, 02 Feb 2022 13:11:34 +01:00
rseq: Remove broken uapi field layout on 32-bit little endian
The rseq rseq_cs.ptr.{ptr32,padding} uapi endianness handling is
entirely wrong on 32-bit little endian: a preprocessor logic mistake
wrongly uses the big endian field layout on 32-bit little endian
architectures.
Fortunately, those ptr32 accessors were never used within the kernel,
and only meant as a convenience for user-space.
Remove those and replace the whole rseq_cs union by a __u64 type, as
this is the only thing really needed to express the ABI. Document how
32-bit architectures are meant to interact with this field.
Fixes: ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union, update includes")
Signed-off-by: Mathieu Desnoyers <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
include/uapi/linux/rseq.h | 20 ++++----------------
kernel/rseq.c | 8 ++++----
2 files changed, 8 insertions(+), 20 deletions(-)
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index 9a402fd..77ee207 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -105,23 +105,11 @@ struct rseq {
* Read and set by the kernel. Set by user-space with single-copy
* atomicity semantics. This field should only be updated by the
* thread which registered this data structure. Aligned on 64-bit.
+ *
+ * 32-bit architectures should update the low order bits of the
+ * rseq_cs field, leaving the high order bits initialized to 0.
*/
- union {
- __u64 ptr64;
-#ifdef __LP64__
- __u64 ptr;
-#else
- struct {
-#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined(__BIG_ENDIAN)
- __u32 padding; /* Initialized to zero. */
- __u32 ptr32;
-#else /* LITTLE */
- __u32 ptr32;
- __u32 padding; /* Initialized to zero. */
-#endif /* ENDIAN */
- } ptr;
-#endif
- } rseq_cs;
+ __u64 rseq_cs;
/*
* Restartable sequences flags field.
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 6d45ac3..97ac20b 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -128,10 +128,10 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
int ret;
#ifdef CONFIG_64BIT
- if (get_user(ptr, &t->rseq->rseq_cs.ptr64))
+ if (get_user(ptr, &t->rseq->rseq_cs))
return -EFAULT;
#else
- if (copy_from_user(&ptr, &t->rseq->rseq_cs.ptr64, sizeof(ptr)))
+ if (copy_from_user(&ptr, &t->rseq->rseq_cs, sizeof(ptr)))
return -EFAULT;
#endif
if (!ptr) {
@@ -217,9 +217,9 @@ static int clear_rseq_cs(struct task_struct *t)
* Set rseq_cs to NULL.
*/
#ifdef CONFIG_64BIT
- return put_user(0UL, &t->rseq->rseq_cs.ptr64);
+ return put_user(0UL, &t->rseq->rseq_cs);
#else
- if (clear_user(&t->rseq->rseq_cs.ptr64, sizeof(t->rseq->rseq_cs.ptr64)))
+ if (clear_user(&t->rseq->rseq_cs, sizeof(t->rseq->rseq_cs)))
return -EFAULT;
return 0;
#endif