Re: [PATCH v2 0/8] Extensible rseq integration

[Rich Felker <dalias@libc.org>]

On Tue, Dec 07, 2021 at 01:59:26PM +0100, Florian Weimer via Libc-alpha wrote:
> This series integrates the previous posted v2 for <thread_pointer.h>.
> 
> It incorporates Mathieu's and Paul E. McKenney suggestion to use a
> volatile read for rseq_abi.cpu_id access, using a new
> THREAD_GETMEM_VOLATILE macro.
> 
> The last patch in the series makes rseq registration consistent across
> threads.
> 
> Florian Weimer (8):
>   nptl: Add <thread_pointer.h> for defining __thread_pointer
>   nptl: Introduce <tcb-access.h> for THREAD_* accessors
>   nptl: Introduce THREAD_GETMEM_VOLATILE
>   nptl: Add rseq registration
>   Linux: Use rseq to accelerate sched_getcpu
>   nptl: Add glibc.pthread.rseq tunable to control rseq registration
>   nptl: Add public rseq symbols and <sys/rseq.h>
>   nptl: rseq failure after registration on main thread is fatal

I'm sorry for bringing this up so late; I wasn't aware that redesign
of the rseq ABI was taking place. I wish this had been discussed in a
cross-libc venue, since, in its current form, I don't think the ABI is
suitable for inclusion in, or use as a third-party library with, musl.

The most pressing issue I see is that it does not admit lazy
registration, which precludes it being implemented outside of libc
(because it has to hook into pthread_create) and imposes runtime cost
on programs which do not use it. RSEQ_CPU_ID_UNINITIALIZED exists to
inform the application about an uninitialized state, but the
application has no way to request an attempt at registration upon
seeing it. I think that would be easy to add. Basically it's just
making the syscall, which a consumer of the ABI could in theory do
itself, but it's probably best not to have it do that and instead have
registration mediated through the ABI/through libc.

Related to this, if rseq is implemented outside of libc, I'm not sure
if there's a safe way to ensure it's unregistered prior to thread
exit. It may already be possible but I haven't sufficiently convinced
myself.

On another issue, while this isn't entirely a show-stopper, I'm not a
fan of requiring constant __rseq_offset. This comes across as an
instance-specific hack to make up for GD TLS being slow, when we
already have a fully general solution to that which isn't being
deployed: TLSDESC. As it stands in the current ABI, whatever library
is providing rseq must be present at application startup; it can't be
dlopened. And due to the ABI this applies *even if* we just wanted to
make rseq always-fail in that case. The ABI simply doesn't admit not
having memory pre-reserved for every thread (note: the size is
something like a +30% increase to musl's per-thread memory usage and
will surely increase over time, which is a lot for something we don't
expect the vast majority of applications to use).

One minor and hopefully non-controversial declared-ABI issue I see is
that the __rseq_offset etc. objects are declared const, with a
pre-relro access hack used to modify them at runtime. This is
incompatible with LTO and static linking. If protecting them is
desired, they should be declared non-const but live in non-modifiable
memory, like string literals do. Otherwise a static linking LTO
compiler is free to copy the initial values directly into code.

I'm not sure what the right thing to do on the verge of release is. If
it were my choice, I would hold it back and wait until it was better
reviewed and these issues worked out before making it public API/ABI,
but I don't know what glibc's constraints here are and how to best
weigh them against the ability to revise this ABI after release. Most
of these things I think *are* of the sort that can be fixed in
non-breaking ways, except that applications written to the current
version might need to adjust before they can use a version of the
API/ABI we'd be willing to adopt in musl.

Rich