[RFC PATCH] rseq: Fix broken uapi field layout on 32-bit little endian

[RFC PATCH] rseq: Fix broken uapi field layout on 32-bit little endian

[Mathieu Desnoyers]

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.

While working on fixing the ppc32 support in librseq [1], I made sure
all 32-bit little endian architectures stopped depending on little
endian byte ordering by using the ptr32 field. It led me to discover
this wrong ptr32 field ordering on little endian.

Because it is already exposed as a UAPI, all we can do for the existing
fields is document the wrong behavior and encourage users to use
alternative mechanisms.

Introduce a new rseq_cs.arch field with correct field ordering. Use this
opportunity to improve the layout so accesses to architecture fields on
both 32-bit and 64-bit architectures are done through the same field
hierarchy, which is much nicer than the previous scheme.

The intended use is now:

* rseq_thread_area->rseq_cs.ptr64: Access the 64-bit value of the rseq_cs
				   pointer. Available on all
                                   architectures (unchanged).

* rseq_thread_area->rseq_cs.arch.ptr: Access the architecture specific
				      layout of the rseq_cs pointer. This
				      is a 32-bit field on 32-bit
				      architectures, and a 64-bit field on
                                      64-bit architectures.

Link: https://git.kernel.org/pub/scm/libs/librseq/librseq.git/ [1]
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 | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index 9a402fdb60e9..68f61cdb45db 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -108,6 +108,12 @@ struct rseq {
 	 */
 	union {
 		__u64 ptr64;
+
+		/*
+		 * The "ptr" field layout is broken on little-endian
+		 * 32-bit architectures due to wrong preprocessor logic.
+		 * DO NOT USE.
+		 */
 #ifdef __LP64__
 		__u64 ptr;
 #else
@@ -121,6 +127,23 @@ struct rseq {
 #endif /* ENDIAN */
 		} ptr;
 #endif
+
+		/*
+		 * The "arch" field provides architecture accessor for
+		 * the ptr field based on architecture pointer size and
+		 * endianness.
+		 */
+		struct {
+#ifdef __LP64__
+			__u64 ptr;
+#elif defined(__BYTE_ORDER) ? (__BYTE_ORDER == __BIG_ENDIAN) : defined(__BIG_ENDIAN)
+			__u32 padding;		/* Initialized to zero. */
+			__u32 ptr;
+#else
+			__u32 ptr;
+			__u32 padding;		/* Initialized to zero. */
+#endif
+		} arch;
 	} rseq_cs;
 
 	/*
-- 
2.17.1

Re: [RFC PATCH] rseq: Fix broken uapi field layout on 32-bit little endian

[Greg KH]

On Sun, Jan 23, 2022 at 02:31:54PM -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.
> 
> While working on fixing the ppc32 support in librseq [1], I made sure
> all 32-bit little endian architectures stopped depending on little
> endian byte ordering by using the ptr32 field. It led me to discover
> this wrong ptr32 field ordering on little endian.
> 
> Because it is already exposed as a UAPI, all we can do for the existing
> fields is document the wrong behavior and encourage users to use
> alternative mechanisms.
> 
> Introduce a new rseq_cs.arch field with correct field ordering. Use this
> opportunity to improve the layout so accesses to architecture fields on
> both 32-bit and 64-bit architectures are done through the same field
> hierarchy, which is much nicer than the previous scheme.
> 
> The intended use is now:
> 
> * rseq_thread_area->rseq_cs.ptr64: Access the 64-bit value of the rseq_cs
> 				   pointer. Available on all
>                                    architectures (unchanged).
> 
> * rseq_thread_area->rseq_cs.arch.ptr: Access the architecture specific
> 				      layout of the rseq_cs pointer. This
> 				      is a 32-bit field on 32-bit
> 				      architectures, and a 64-bit field on
>                                       64-bit architectures.
> 
> Link: https://git.kernel.org/pub/scm/libs/librseq/librseq.git/ [1]
> 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 | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [RFC PATCH] rseq: Fix broken uapi field layout on 32-bit little endian

[Linus Torvalds]

On Sun, Jan 23, 2022 at 9:32 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> 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.

Please don't double down on something that was already broken once.

Just remove the broken 32-bit one entirely that the kernel doesn't
even use, and make everybody use

   __u64 ptr64;

and be done with it.

Adding a new "arch.ptr32" thing to replace the broken ptr.ptr32 is
just not worth it. This "convenience feature" never worked correctly
on any relevant architecture, so it clearly was never a convenience
feature, and deciding to try to re-do it because it was broken and
pointless the first time around isn't sane.

The definition of insanity is literally to do the same broken thing over again.

So just remove the broken ptr.ptr32 thing, don't add anything new to
replace it. Existing binaries will continue to work (or not work) as
well as they ever did. And new people getting new headers will get a
clear and proper compile error for the broken code that they can
trivially fix using 'ptr64' after they have actually thought about it
for a while.

Giving them a "arch.ptr32" doesn't help them at all. Quite the
reverse. You seem to hve the intention that they should just
mindlessly replace "ptr.ptr32" with "arch.ptr32", and now their code
won't actually work the same. Plus it will build with one version but
not the other.

In contrast, if you just tell people "ptr.ptr32 was always broken, use
ptr64 instead", it will actually work THE SAME with both old and new
headers. No odd "changed behavior from syntactic patch". No odd "this
won't work with older headers so now you have to add some
configuration or #ifdef".

The kernel cares about maintaining the ABI. The *binary* interface. If
the API was broken, it needs to be fixed. Not made worse by keeping
the broken fields and adding new ones for no reason.

               Linus

Re: [RFC PATCH] rseq: Fix broken uapi field layout on 32-bit little endian

[Mathieu Desnoyers]

----- On Jan 24, 2022, at 2:42 AM, Linus Torvalds torvalds@linux-foundation.org wrote:

> On Sun, Jan 23, 2022 at 9:32 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> 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.
> 
> Please don't double down on something that was already broken once.
> 
> Just remove the broken 32-bit one entirely that the kernel doesn't
> even use, and make everybody use
> 
>   __u64 ptr64;
> 
> and be done with it.

OK, should I just leave:

struct rseq {
  [...]
  union rseq_cs {
    __u64 ptr64;
  } rseq_cs;
  [...]
};

and remove all the other content from the union, so users of
rseq_abi->rseq_cs.ptr64 will continue to work as-is with either
old and new headers ? This keeps a union in place with a single
element, so I just want to confirm with you that is what you
have in mind.

It does make tons of sense to just remove the broken convenience
code and let user-space handle this based on the ptr64 field, so
it will work fine with old and new headers.

Thanks for your feedback, and travel safe!

Mathieu

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