Re: [RFC PATCH glibc 4/8] glibc: Perform rseq(2) registration at C startup and thread creation (v15)

* Mathieu Desnoyers:

----- On Mar 19, 2020, at 2:16 PM, Florian Weimer fw@deneb.enyo.de wrote:

quoted

* Mathieu Desnoyers:

quoted

quoted

You also need to add an assert that the compiler supports
__attribute__ ((aligned)) because ignoring it produces an
ABI-incompatible header.

Are you aware of some helper macro I should use to do this, or
is it done elsewhere in glibc ?

I don't think we have any such GCC-only types yet.  max_align_t is
provided by GCC itself.

I was thinking of adding the following to

sysdeps/unix/sysv/linux/rseq-internal.h: rseq_register_current_thread()

+  /* Ensure the compiler supports __attribute__ ((aligned)).  */
+  _Static_assert (__alignof__ (struct rseq_cs) >= 4 * sizeof(uint64_t),
+                 "alignment");
+  _Static_assert (__alignof__ (struct rseq) >= 4 * sizeof(uint64_t),
+                 "alignment");
+

Something like it would have to go into the *public* header.

Inside glibc, you can assume __attribute__ support.

quoted

quoted

quoted

The struct rseq/struct rseq_cs definitions
are broken, they should not try to change the alignment.

AFAIU, this means we should ideally not have used __attribute__((aligned))
in the uapi headers in the first place. Why is it broken ?

Compilers which are not sufficiently GCC-compatible define
__attribute__(X) as the empty expansion, so you silently get a
different ABI.

It is worth noting that rseq.h is not the only Linux uapi header
which uses __attribute__ ((aligned)), so this ABI problem exists today
anyway for those compilers.

Yuck.  Even with larger-than-16 alignment?

quoted

There is really no need to specify 32-byte alignment here.  Is not
even the size of a standard cache line.  It can result in crashes if
these structs are heap-allocated using malloc, when optimizing for
AVX2.

Why would it be valid to allocate those with malloc ? Isn't it the
purpose of posix_memalign() ?

It would not be valid, but I don't think we have diagnostics for C
like we have them for C++'s operator new.

quoted

quoted

However, now that it is in the wild, it's a bit late to change that.

I had forgotten about the alignment crashes.  I think we should
seriously consider changing the types. 8-(

I don't think this is an option at this stage given that it is part
of the Linux kernel UAPI. I am not convinced that it is valid at all
to allocate struct rseq or struct rseq_cs with malloc(), because it
does not guarantee any alignment.

The kernel ABI doesn't change.  The kernel cannot use the alignment
information anyway.  Userspace struct layout may change in subtle
ways, though.