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

[Carlos O'Donell <carlos@redhat.com>]

On 2/1/22 11:36, Florian Weimer wrote:
> * Rich Felker:
> 
>> 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.
> 
> Well, I Cc:ed you on the original proposal in November, and cross-posted
> it to linux-api as well.
> 
>> 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.
> 
> I rejected that because the programming model is too complex: In the
> extreme, a library that observes rseq support on the main thread may be
> called again from another thread where rseq is not yet enabled, and
> cannot be enabled.
> 
> I think it is also necessary to enable it unconditionally to force
> people to actually implement support for it in their tools (e.g., CRIU).
> Otherwise we'll never get to the point where it is reliable.  I doubt
> we'd have learned about the CRIU issue by now unless we took that step.

Agreed.

>> 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.
> 
> I expect that asking for rseq to be implemented outside of libc is like
> asking for robust mutexes to be implemented outside libc: it's really
> pushing what can be done in a library.

This is a design decision that we made in glibc.

>> 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).
> 
> If the memory is not allocated, __rseq_size can be set to 0.
> 
>> 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.
> 
> Yes, you'll need a compiler barrier with LTO.  It's not different from
> other types of relocations.

Agreed.

At the language level the offset is constant.

Of the two choices, I think that making __rseq_offset non-const is pessimistic.

LTO and static linking must be aware of details outside of the language level
and may need to handle those details in an implementation defined manner.

>> 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.
> 
> Quoting for Mathieu's benefit.  Also Cc:ing Carlos as the release
> manager.

We have spent ~1.5 years correcting rseq integration since the initial attempt
in July 2020. The inclusion of __rseq is ready for glibc 2.35.

I plan to make the release with the ABI included.

I think we can continue to work with the musl community on integration issues.

-- 
Cheers,
Carlos.