[PATCH] s390: Prevent hoisting the thread-pointer read in THREAD_SET_STACK_GUARD (BZ 34297)

[Adhemerval Zanella Netto]

On 06/07/26 09:50, Stefan Liebler wrote:
> On 6/17/26 16:25, Adhemerval Zanella wrote:
>> THREAD_SET_STACK_GUARD reads the thread pointer via THREAD_SELF
>> (__builtin_thread_pointer), which the compiler treats as invariant.  In the
>> static startup path the thread pointer is installed by the __libc_setup_tls
>> call that immediately precedes the guard store, so the read must stay below
>> it.  The existing barrier only clobbered the access registers a0/a1, which
>> creates no dependency on the call, so the compiler could move the whole
>> barrier and read above __libc_setup_tls.
>>
>> This is sensitive to instruction scheduling and recent TLS startup
>> changes exposed it on s390x.
>>
>> Add a "memory" clobber to the barrier so it is tied to the call's memory
>> effects and cannot be hoisted above it.  The macro is shared with the
>> dynamic loader, so both startup paths are covered.
>>
>> I checked on s390x-linux-gnu build for arch5, arch8, arch9, and arch11
>> byt running the elf tests on qemu system (kernel 6.1.0).
>> ---
>>  sysdeps/s390/nptl/tls.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/s390/nptl/tls.h b/sysdeps/s390/nptl/tls.h
>> index 41fd473d14a..6ecaa243841 100644
>> --- a/sysdeps/s390/nptl/tls.h
>> +++ b/sysdeps/s390/nptl/tls.h
>> @@ -131,7 +131,7 @@ typedef struct
>>  #define THREAD_SET_STACK_GUARD(value) \
>>    do									      \
>>     {									      \
>> -     __asm__ __volatile__ ("" : : : "a0", "a1");			      \
>> +     __asm__ __volatile__ ("" : : : "a0", "a1", "memory");		      \
>>       THREAD_SETMEM (THREAD_SELF, header.stack_guard, value);		      \
>>     }									      \
>>    while (0)
> 
> Hi Adhemerval,
> 
> sorry for the long delay. I was away quite away for a while.
> 
> I can confirm, that without the patch for a static binary, reading a0 is
> moved before the __libc_setup_tls call where a0/a1 is set up to the
> valid thread pointer.
> 
> With the additional memory barrier, the reading of a0 takes place after
> __libc_setup_tls.
> 
> I've also tested this patch with various configurations.
> 
> Reviewed-by: Stefan Liebler <stli@linux.ibm.com>

Thanks.

> 
> 
> 
> Independent of this patch, I've recognized that e.g. the new static test
> elf/tst-ptrguard-static-dlopen can fail as test-container may fail to
> start due to wrong GLIBC_2.XY version of system libc.so symbols. Usually
> test-via-rtld-prefix is set to run-via-rtld-prefix in Makeconfig. The
> latter one is set to "../ld.so --library-path ..." for dynamic tests,
> but not for those listed in tests-static. In this case, the test itself
> is static, but test-container is dynamically linked and is run without
> "../ld.so --library-path ...".
Yes, Sachin has already brought this up and master should have this fixed [1].

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=1ab000387207a968f7cd18a5f3e3e7dcdadad139