Re: [PATCH] aarch64: Remove ld.so __tls_get_addr plt usage

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 09/04/24 05:30, Szabolcs Nagy wrote:
> The 04/08/2024 13:57, Adhemerval Zanella Netto wrote:
>> On 08/04/24 04:26, Szabolcs Nagy wrote:
>>> The 04/07/2024 16:29, Cristian Rodríguez wrote:
>>>> On Fri, Apr 5, 2024 at 11:59 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>>>>> The 04/05/2024 09:35, Adhemerval Zanella wrote:
>>>>>> Use the hidden alias instead.
>>>>>>
>>>>>> Checked on aarch64-linux-gnu.
>>>>>
>>>>> does this change behaviour in case __tls_get_addr is interposed?
>>>>
>>>> Wut ? is that really supported.. I mean.. isn't that symbol prefix
>>>> reserved for the implementation and any assumption about it is either
>>>> ID or UB?
>>>
>>> a behaviour can change even if it's not supported.
>>> i did not try to imply that it should be supported.
>>>
>>> i know sanitizers interpose __tls_get_addr, because
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=16291
>>> i don't know if that hack works at all now for tlsdesc
>>> (where the ld.so calls __tls_get_addr, not user code)
>>>
>>> my question was if we investigated this issue since it
>>> is useful to document then in the commit msg (or news
>>> entry if this affects users)
>>
>> This change 'breaks' the sanitizer trick to get the dynamic TLS, with
>> this patch I now see:
>>
>>   MemorySanitizer-AARCH64 :: dtls_test.c
>>   SanitizerCommon-asan-aarch64-Linux :: Linux/resize_tls_dynamic.cpp
>>   SanitizerCommon-msan-aarch64-Linux :: Linux/resize_tls_dynamic.cpp
>>   SanitizerCommon-tsan-aarch64-Linux :: Linux/resize_tls_dynamic.cpp
>>
>> And it does not fail on x86 only because it uses -mtls=gnu as default
>> (the same tests fail on x86 with -mtls=gnu2).
>>
>> Now that GCC and distributions are aiming to use GNU2/DESC as the
>> default TLS, this hack will also break on x86.  So the question is 
>> whether we revert 050f7298e1ecc39887c329037575ccd972071255 and 
>> document that __tls_get_addr should be interposable, or move with this 
>> change and try to come up with a possible solution for BZ#16291.
>>
>> I bringing this because we will have another two ABIs with tlsdesc
>> support (loongarch and riscv).
> 
> adding some sanitizer committers to cc.
> 
> tl;dr: in the next glibc release tlsdesc will not call
> __tls_get_addr in an interposable way in the dynamic tls
> allocation case, unless somebody screems that this is needed.
> (affects targets that may default to tlsdesc, but note that
> the dynamic case only triggers with tlsdesc when a lot of
> dlopened tls is used, otherwise static tls area is used)

Just a note that this already true for x86 with -mtls=gnu2 since
2.21.  And now that distro are aiming to make it default, this issues
will happen more often.

> 
> i think it is also possible that we will use custom malloc
> in ld.so which may be just as big change for the sanitizers.
> (this can make tls access signal safe)
> 
> i'm not against the change, but if we plan to add several
> interposable hooks as in
> https://sourceware.org/glibc/wiki/ThreadPropertiesAPI
> then we might as well keep __tls_get_addr PLT for now.
> 

I don't have a strong opinion, but what I really want is to have
consistency over the architectures.  Meaning that if we want to keep
the __tls_get_addr PLT for sanitizer/runtime hooks, it would be good
to revert the x86 change.

It also means to document it properly somewhere and make the new
RISC-V and loongarch follow the same guidelines.

I will take a look again on the ThreadPropertiesAPI, since it is has
been more and more a demanding issue.