Re: [RFC PATCH 02/15] rseq: Remove broken uapi field layout on 32-bit little endian

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- 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


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