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

[Mathieu Desnoyers]

----- On Jan 26, 2022, at 12:16 PM, David Laight David.Laight@xxxxxxxxxx wrote:

> From: Mathieu Desnoyers
>> Sent: 25 January 2022 19:01
>> 
>> ----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers
>> mathieu.desnoyers@xxxxxxxxxxxx wrote:
>> 
>> > ----- On Jan 25, 2022, at 7:21 AM, Christian Brauner brauner@xxxxxxxxxx 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