Re: Question about s390 THREAD_SET_STACK_GUARD

[Stefan Liebler <stli at linux dot ibm dot com>]

On 11/05/2018 04:59 PM, Adhemerval Zanella wrote:
> On 02/11/2018 19:40, Gc Frix wrote:
> > Hello,
> >
> > I was looking through the s390 sources and I noticed something I don't
> > quite understand. From sysdeps/s390/nptl/tls.h:
> >
> > ```
> > /* Set the stack guard field in TCB head.  */
> > #define THREAD_SET_STACK_GUARD(value) \
> >    do \
> >     { \
> >       __asm__ __volatile__ ("" : : : "a0", "a1"); \
> >       THREAD_SETMEM (THREAD_SELF, header.stack_guard, value); \
> >     } \
> >    while (0)
> >
> > /* For reference, here's THREAD_SETMEM and THREAD_SELF.  */
> > #define THREAD_SETMEM(descr, member, value) \
> >    descr->member = (value)
> >
> > # define THREAD_SELF ((struct pthread *) __builtin_thread_pointer ())
> > ```
> >
> > I can't figure out what the point of the asm in THREAD_SET_STACK_GUARD
> > is. I know that for s390x, the thread pointer is split between a0 and
> > a1, and that __builtin_thread_pointer() is a GCC builtin that represents
> > them (__builtin_thread_pointer is completely undocumented for s390, by
> > the way). What I don't understand is why the asm is necessary at all.
> > The most I can figure is that it's meant to force a reload from a0 and
> > a1, but why? Shouldn't they only ever get modified by context switches
> > after they get set?
> >
> > I'm new to glibc, so I apologize if this is a dumb question.
> >
> > Regards,
> > Giancarlo Frix
>
> For s390x the only symbol which the asm volatile seems to interfere is
> security_init from ld.so:
>
> $ diff -u ld-base.disas ld-patched.disas
> --- ld-base.disas       2018-11-05 10:02:30.575798123 -0200
> +++ ld-patched.disas    2018-11-05 10:04:58.812381637 -0200
> @@ -167,19 +167,19 @@
>       1236:      07 07                   nopr    %r7
>
>   0000000000001238 <security_init>:
> -    1238:      c0 40 00 01 36 20       larl    %r4,27e78 <_dl_random>
> -    123e:      e3 30 40 00 00 04       lg      %r3,0(%r4)
> -    1244:      b2 4f 00 10             ear     %r1,%a0
> -    1248:      eb 11 00 20 00 0d       sllg    %r1,%r1,32
> -    124e:      b2 4f 00 11             ear     %r1,%a1
> -    1252:      e3 50 30 00 00 04       lg      %r5,0(%r3)
> -    1258:      a5 54 00 ff             nihh    %r5,255
> -    125c:      c0 20 00 01 32 fe       larl    %r2,27858 <__pointer_chk_guard_local>
> -    1262:      a7 09 00 00             lghi    %r0,0
> -    1266:      e3 50 10 28 00 24       stg     %r5,40(%r1)
> -    126c:      e3 10 30 08 00 04       lg      %r1,8(%r3)
> -    1272:      e3 00 40 00 00 24       stg     %r0,0(%r4)
> -    1278:      e3 10 20 00 00 24       stg     %r1,0(%r2)
> +    1238:      c0 10 00 01 36 20       larl    %r1,27e78 <_dl_random>
> +    123e:      b2 4f 00 20             ear     %r2,%a0
> +    1242:      eb 22 00 20 00 0d       sllg    %r2,%r2,32
> +    1248:      e3 40 10 00 00 04       lg      %r4,0(%r1)
> +    124e:      b2 4f 00 21             ear     %r2,%a1
> +    1252:      c0 30 00 01 33 03       larl    %r3,27858 <__pointer_chk_guard_local>
> +    1258:      e3 50 40 00 00 04       lg      %r5,0(%r4)
> +    125e:      a5 54 00 ff             nihh    %r5,255
> +    1262:      e3 50 20 28 00 24       stg     %r5,40(%r2)
> +    1268:      a7 29 00 00             lghi    %r2,0
> +    126c:      e3 20 10 00 00 24       stg     %r2,0(%r1)
> +    1272:      e3 10 40 08 00 04       lg      %r1,8(%r4)
> +    1278:      e3 10 30 00 00 24       stg     %r1,0(%r3)
>       127e:      07 fe                   br      %r14
>
> - ld-base.disas: master glibc
> - ld-patched.disas: master glibc with the asm removed.
>
> I can't identify any significant change that indeed justify the asm
> requirement neither git log gives any indication why this change
> was added. Stefan and Andreas, do you why and if this is required
> for s390?

Hi,

Martin, who wrote the patch, told us that there was a bug where the 
stack_chk_guard was written to the false location.
But unfortunately I don't have any further information about this bug or 
which gcc version was involved.

It could be that the static functions init_tls and security_init were 
inlined in dl_main:
...
  if (tcbp == NULL)
    tcbp = init_tls ();

  if (__glibc_likely (need_security_init))
    /* Initialize security features.  But only if we have not done it
       earlier.  */
    security_init ();
...

In init_tls, the access registers are set up by TLS_INIT_TP from 
sysdeps/s390/nptl/tls.h which is using __builtin_set_thread_pointer:
# define TLS_INIT_TP(thrdescr) \
  ({ void *_thrdescr = (thrdescr);	\
     tcbhead_t *_head = _thrdescr;      \
				      \
     _head->tcb = _thrdescr;	      \
     /* For now the thread descriptor is at the same address.  */ \
     _head->self = _thrdescr;	      \
     /* New syscall handling support.  */   \
     INIT_SYSINFO;		      \
			      \
    __builtin_set_thread_pointer (_thrdescr);	    \
    NULL;	\
  })


Perhaps there was an bug/optimization in gcc where the asm was needed.
But as we don't know more details about this issue, we decided to not 
remove the asm!

Bye
Stefan