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

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 <[email protected]>



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

Thanks,
Stefan