* 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).