* Question about s390 THREAD_SET_STACK_GUARD
@ 2018-11-02 22:40 Gc Frix
2018-11-05 15:59 ` Adhemerval Zanella
0 siblings, 1 reply; 3+ messages in thread
From: Gc Frix @ 2018-11-02 22:40 UTC (permalink / raw)
To: libc-help
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: Question about s390 THREAD_SET_STACK_GUARD 2018-11-02 22:40 Question about s390 THREAD_SET_STACK_GUARD Gc Frix @ 2018-11-05 15:59 ` Adhemerval Zanella 2018-11-08 14:57 ` Stefan Liebler 0 siblings, 1 reply; 3+ messages in thread From: Adhemerval Zanella @ 2018-11-05 15:59 UTC (permalink / raw) To: libc-help; +Cc: Stefan Liebler, Andreas Krebbel 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? ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Question about s390 THREAD_SET_STACK_GUARD 2018-11-05 15:59 ` Adhemerval Zanella @ 2018-11-08 14:57 ` Stefan Liebler 0 siblings, 0 replies; 3+ messages in thread From: Stefan Liebler @ 2018-11-08 14:57 UTC (permalink / raw) To: Adhemerval Zanella, libc-help 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-08 14:57 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-02 22:40 Question about s390 THREAD_SET_STACK_GUARD Gc Frix 2018-11-05 15:59 ` Adhemerval Zanella 2018-11-08 14:57 ` Stefan Liebler
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).