| Message ID | 20220124171253.22072-3-mathieu.desnoyers@efficios.com |
|---|---|
| State | New |
| Headers | show |
| Series | rseq uapi and selftest updates | expand |
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 <mathieu.desnoyers@efficios.com> > Cc: Florian Weimer <fw@deneb.enyo.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: linux-api@vger.kernel.org > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: Dave Watson <davejwatson@fb.com> > Cc: Paul Turner <pjt@google.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: "H . Peter Anvin" <hpa@zytor.com> > Cc: Andi Kleen <andi@firstfloor.org> > Cc: Christian Brauner <christian.brauner@ubuntu.com> > Cc: Ben Maurer <bmaurer@fb.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Josh Triplett <josh@joshtriplett.org> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Michael Kerrisk <mtk.manpages@gmail.com> > Cc: Joel Fernandes <joelaf@google.com> > Cc: Paul E. McKenney <paulmck@kernel.org> > --- > 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 brauner@kernel.org 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 <mathieu.desnoyers@efficios.com> >> Cc: Florian Weimer <fw@deneb.enyo.de> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: linux-api@vger.kernel.org >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Boqun Feng <boqun.feng@gmail.com> >> Cc: Andy Lutomirski <luto@amacapital.net> >> Cc: Dave Watson <davejwatson@fb.com> >> Cc: Paul Turner <pjt@google.com> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: "H . Peter Anvin" <hpa@zytor.com> >> Cc: Andi Kleen <andi@firstfloor.org> >> Cc: Christian Brauner <christian.brauner@ubuntu.com> >> Cc: Ben Maurer <bmaurer@fb.com> >> Cc: Steven Rostedt <rostedt@goodmis.org> >> Cc: Josh Triplett <josh@joshtriplett.org> >> Cc: Linus Torvalds <torvalds@linux-foundation.org> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> Cc: Michael Kerrisk <mtk.manpages@gmail.com> >> Cc: Joel Fernandes <joelaf@google.com> >> Cc: Paul E. McKenney <paulmck@kernel.org> >> --- >> 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
----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner brauner@kernel.org 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
On Tue, Jan 25, 2022 at 02:00:48PM -0500, Mathieu Desnoyers wrote: > ----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > > > ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner brauner@kernel.org 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 mathieu.desnoyers@efficios.com wrote: > > > ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner brauner@kernel.org 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 David.Laight@ACULAB.COM wrote: > From: Mathieu Desnoyers >> Sent: 25 January 2022 19:01 >> >> ----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers >> mathieu.desnoyers@efficios.com wrote: >> >> > ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner brauner@kernel.org 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)
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 <mathieu.desnoyers@efficios.com> > Cc: Florian Weimer <fw@deneb.enyo.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: linux-api@vger.kernel.org > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: Dave Watson <davejwatson@fb.com> > Cc: Paul Turner <pjt@google.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: "H . Peter Anvin" <hpa@zytor.com> > Cc: Andi Kleen <andi@firstfloor.org> > Cc: Christian Brauner <christian.brauner@ubuntu.com> > Cc: Ben Maurer <bmaurer@fb.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Josh Triplett <josh@joshtriplett.org> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Michael Kerrisk <mtk.manpages@gmail.com> > Cc: Joel Fernandes <joelaf@google.com> > Cc: Paul E. McKenney <paulmck@kernel.org> > --- Looks way cleaner now! Fwiw, Acked-by: Christian Brauner <brauner@kernel.org> > 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 >
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; /*
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 <mathieu.desnoyers@efficios.com> Cc: Florian Weimer <fw@deneb.enyo.de> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: linux-api@vger.kernel.org Cc: Peter Zijlstra <peterz@infradead.org> Cc: Boqun Feng <boqun.feng@gmail.com> Cc: Andy Lutomirski <luto@amacapital.net> Cc: Dave Watson <davejwatson@fb.com> Cc: Paul Turner <pjt@google.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: "H . Peter Anvin" <hpa@zytor.com> Cc: Andi Kleen <andi@firstfloor.org> Cc: Christian Brauner <christian.brauner@ubuntu.com> Cc: Ben Maurer <bmaurer@fb.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Josh Triplett <josh@joshtriplett.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Michael Kerrisk <mtk.manpages@gmail.com> Cc: Joel Fernandes <joelaf@google.com> Cc: Paul E. McKenney <paulmck@kernel.org> --- include/uapi/linux/rseq.h | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)