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

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

* Mathieu Desnoyers:

quoted

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

OK, so the _Static_assert () could sit in sys/rseq.h

quoted

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?

There are two:

target_core_user.h
45:#define ALIGN_SIZE 64 /* Should be enough for most CPUs */
58:	__u32 cmd_tail __attribute__((__aligned__(ALIGN_SIZE)));

netfilter_bridge/ebtables.h:90:	char data[0] __attribute__ ((aligned (__alignof__(struct ebt_replace))));
netfilter_bridge/ebtables.h:132:	unsigned char data[0] __attribute__ ((aligned (__alignof__(struct ebt_replace))));
netfilter_bridge/ebtables.h:145:	unsigned char data[0] __attribute__ ((aligned (__alignof__(struct ebt_replace))));
netfilter_bridge/ebtables.h:158:	unsigned char data[0] __attribute__ ((aligned (__alignof__(struct ebt_replace))));
netfilter_bridge/ebtables.h:191:	unsigned char elems[0] __attribute__ ((aligned (__alignof__(struct ebt_replace))));

quoted

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.

We could at least make an effort to let people know that alignment is
required here when allocating struct rseq and struct rseq_cs on the
heap by adding some comments to that effect in linux/rseq.h ?

quoted

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.

Considering the amount of pain this can cause in user-space, and because
it can break userspace, this is not a UAPI change I am willing to consider.
I'm not sure why we are even discussing the possibility of breaking a Linux
UAPI considering that those are set in stone.

Thanks,

Mathieu

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