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

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

From: Mathieu Desnoyers

quoted

Sent: 25 January 2022 19:01

----- On Jan 25, 2022, at 9:41 AM, Mathieu Desnoyers
mathieu.desnoyers@efficios.com wrote:

quoted

----- On Jan 25, 2022, at 7:21 AM, Christian Brauner brauner@kernel.org wrote:

[...]

quoted

quoted

quoted

 include/uapi/linux/rseq.h | 17 ++++-------------

[...]

quoted

quoted

quoted

 	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.

quoted

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