Patchwork Add __private_ss to s390 struct tcbhead.

login
register
mail settings
Submitter Marcin Kościelnicki
Date Jan. 2, 2016, 4:34 p.m.
Message ID <1451752466-9429-1-git-send-email-koriakin@0x04.net>
Download mbox | patch
Permalink /patch/10197/
State New
Headers show

Comments

Marcin Kościelnicki - Jan. 2, 2016, 4:34 p.m.
Preparation for gcc -fsplit-stack support (gcc bug #68191).  The new
field is basically identical to the one on x86.  Its TCB offset needs
to be constant, as it'll be hardcoded in gcc.

	* sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
---
 ChangeLog               | 4 ++++
 sysdeps/s390/nptl/tls.h | 4 ++++
 2 files changed, 8 insertions(+)
Florian Weimer - Jan. 4, 2016, 7:08 a.m.
On 01/02/2016 05:34 PM, Marcin Kościelnicki wrote:
> Preparation for gcc -fsplit-stack support (gcc bug #68191).  The new
> field is basically identical to the one on x86.  Its TCB offset needs
> to be constant, as it'll be hardcoded in gcc.

I thought split stacks were considered a failure by the Go team?  Do you
have another use case which does not suffer from the problems they
identified?

Florian
Stefan Liebler - Jan. 12, 2016, 12:21 p.m.
This is okay for s390.

Adhemerval, is this okay for 2.23?

It is needed to enable split-stack support on s390 (See gcc bug #68191).

Bye Stefan

On 01/02/2016 05:34 PM, Marcin Kościelnicki wrote:
> Preparation for gcc -fsplit-stack support (gcc bug #68191).  The new
> field is basically identical to the one on x86.  Its TCB offset needs
> to be constant, as it'll be hardcoded in gcc.
>
> 	* sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
> ---
>   ChangeLog               | 4 ++++
>   sysdeps/s390/nptl/tls.h | 4 ++++
>   2 files changed, 8 insertions(+)
>
> diff --git a/ChangeLog b/ChangeLog
> index 8c339d2..e39e2ce 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2015-01-02  Marcin Kościelnicki  <koriakin@0x04.net>
> +
> +	* sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
> +
>   2016-01-01  Mike Frysinger  <vapier@gentoo.org>
>
>   	[BZ #15421]
> diff --git a/sysdeps/s390/nptl/tls.h b/sysdeps/s390/nptl/tls.h
> index 28a2119..0e61154 100644
> --- a/sysdeps/s390/nptl/tls.h
> +++ b/sysdeps/s390/nptl/tls.h
> @@ -53,7 +53,11 @@ typedef struct
>     int gscope_flag;
>   #ifndef __ASSUME_PRIVATE_FUTEX
>     int private_futex;
> +#else
> +  int __glibc_reserved1;
>   #endif
> +  /* GCC split stack support.  */
> +  void *__private_ss;
>   } tcbhead_t;
>
>   # ifndef __s390x__
>
Adhemerval Zanella Netto - Jan. 12, 2016, 12:39 p.m.
Yes, the initial patch was sent prior the release dates. The only bit I 
would some comments is why adding '__glibc_reserved1' fields (alignment
maybe?).

On 12-01-2016 10:21, Stefan Liebler wrote:
> This is okay for s390.
> 
> Adhemerval, is this okay for 2.23?
> 
> It is needed to enable split-stack support on s390 (See gcc bug #68191).
> 
> Bye Stefan
> 
> On 01/02/2016 05:34 PM, Marcin Kościelnicki wrote:
>> Preparation for gcc -fsplit-stack support (gcc bug #68191).  The new
>> field is basically identical to the one on x86.  Its TCB offset needs
>> to be constant, as it'll be hardcoded in gcc.
>>
>>     * sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
>> ---
>>   ChangeLog               | 4 ++++
>>   sysdeps/s390/nptl/tls.h | 4 ++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index 8c339d2..e39e2ce 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,7 @@
>> +2015-01-02  Marcin Kościelnicki  <koriakin@0x04.net>
>> +
>> +    * sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
>> +
>>   2016-01-01  Mike Frysinger  <vapier@gentoo.org>
>>
>>       [BZ #15421]
>> diff --git a/sysdeps/s390/nptl/tls.h b/sysdeps/s390/nptl/tls.h
>> index 28a2119..0e61154 100644
>> --- a/sysdeps/s390/nptl/tls.h
>> +++ b/sysdeps/s390/nptl/tls.h
>> @@ -53,7 +53,11 @@ typedef struct
>>     int gscope_flag;
>>   #ifndef __ASSUME_PRIVATE_FUTEX
>>     int private_futex;
>> +#else
>> +  int __glibc_reserved1;
>>   #endif
>> +  /* GCC split stack support.  */
>> +  void *__private_ss;
>>   } tcbhead_t;
>>
>>   # ifndef __s390x__
>>
>
Carlos O'Donell - Jan. 13, 2016, 2:41 a.m.
On 01/02/2016 11:34 AM, Marcin Kościelnicki wrote:
> Preparation for gcc -fsplit-stack support (gcc bug #68191).  The new
> field is basically identical to the one on x86.  Its TCB offset needs
> to be constant, as it'll be hardcoded in gcc.
> 
> 	* sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.

What happens if you run newly compiled code with split-stack support on
a glibc that doesn't include this space allocated in tcbhead_t? You get
a write beyond the tcbhead_t into some other data? Depending on the thread
memory layout that could be a guard page or static TLS data?

This is the same problem we saw in POWER when adding a field in tcbhead_t
for fast-architecture access.

The only way to do this compatibly is to add a versioned symbol that
the compiler references to in order to prevent new binaries from
running on old glibc and crashing or worse silently corrupting data.

To reiterate: How do you plan to handle compatibility for this new feature?

If you did an analysis of nptl/allocatestack.c and showed that there were
still alignment bytes left, that might be one way to work around this without
needing a versioned reference.

Cheers,
Carlos.

> ---
>  ChangeLog               | 4 ++++
>  sysdeps/s390/nptl/tls.h | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 8c339d2..e39e2ce 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2015-01-02  Marcin Kościelnicki  <koriakin@0x04.net>
> +
> +	* sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
> +
>  2016-01-01  Mike Frysinger  <vapier@gentoo.org>
>  
>  	[BZ #15421]
> diff --git a/sysdeps/s390/nptl/tls.h b/sysdeps/s390/nptl/tls.h
> index 28a2119..0e61154 100644
> --- a/sysdeps/s390/nptl/tls.h
> +++ b/sysdeps/s390/nptl/tls.h
> @@ -53,7 +53,11 @@ typedef struct
>    int gscope_flag;
>  #ifndef __ASSUME_PRIVATE_FUTEX
>    int private_futex;
> +#else
> +  int __glibc_reserved1;
>  #endif
> +  /* GCC split stack support.  */
> +  void *__private_ss;
>  } tcbhead_t;
>  
>  # ifndef __s390x__
>
Marcin Kościelnicki - Jan. 13, 2016, 7:52 a.m.
On 13/01/16 03:41, Carlos O'Donell wrote:
> On 01/02/2016 11:34 AM, Marcin Kościelnicki wrote:
>> Preparation for gcc -fsplit-stack support (gcc bug #68191).  The new
>> field is basically identical to the one on x86.  Its TCB offset needs
>> to be constant, as it'll be hardcoded in gcc.
>>
>> 	* sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
>
> What happens if you run newly compiled code with split-stack support on
> a glibc that doesn't include this space allocated in tcbhead_t? You get
> a write beyond the tcbhead_t into some other data? Depending on the thread
> memory layout that could be a guard page or static TLS data?
>
> This is the same problem we saw in POWER when adding a field in tcbhead_t
> for fast-architecture access.
>
> The only way to do this compatibly is to add a versioned symbol that
> the compiler references to in order to prevent new binaries from
> running on old glibc and crashing or worse silently corrupting data.
>
> To reiterate: How do you plan to handle compatibility for this new feature?
>
> If you did an analysis of nptl/allocatestack.c and showed that there were
> still alignment bytes left, that might be one way to work around this without
> needing a versioned reference.
>

I have added this field at the end of tcbhead_t, which is part of struct 
pthread, defined here: 
https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/descr.h;h=8e4938deb5311a325d6b67fbc3041037110c5f9e;hb=HEAD#l129 
. The tcbhead_t is stuffed there inside a union with void 
*__padding[24], which is much longer than current tcbhead_t. Thus 
applying this patch to glibc won't even change anything in compiler 
output - there's always been space there, and this patch merely reserves 
it for split-stack use.

The situation is different for POWER, since it has TLS_DTV_AT_TP - in 
this case, there's no padding for tcbhead_t and no extra expension space.

Marcin Kościelnicki
Andreas Schwab - Jan. 13, 2016, 8:54 a.m.
Marcin Kościelnicki <koriakin@0x04.net> writes:

> diff --git a/sysdeps/s390/nptl/tls.h b/sysdeps/s390/nptl/tls.h
> index 28a2119..0e61154 100644
> --- a/sysdeps/s390/nptl/tls.h
> +++ b/sysdeps/s390/nptl/tls.h
> @@ -53,7 +53,11 @@ typedef struct
>    int gscope_flag;
>  #ifndef __ASSUME_PRIVATE_FUTEX
>    int private_futex;
> +#else
> +  int __glibc_reserved1;
>  #endif

You can keep the name the same in all cases.  This is an internal
header, so no namespace control.

> +  /* GCC split stack support.  */
> +  void *__private_ss;

Similarily, no need for namespace control.

Andreas.
Marcin Kościelnicki - Jan. 13, 2016, 10:05 a.m.
On 13/01/16 09:54, Andreas Schwab wrote:
> Marcin Kościelnicki <koriakin@0x04.net> writes:
>
>> diff --git a/sysdeps/s390/nptl/tls.h b/sysdeps/s390/nptl/tls.h
>> index 28a2119..0e61154 100644
>> --- a/sysdeps/s390/nptl/tls.h
>> +++ b/sysdeps/s390/nptl/tls.h
>> @@ -53,7 +53,11 @@ typedef struct
>>     int gscope_flag;
>>   #ifndef __ASSUME_PRIVATE_FUTEX
>>     int private_futex;
>> +#else
>> +  int __glibc_reserved1;
>>   #endif
>
> You can keep the name the same in all cases.  This is an internal
> header, so no namespace control.
>
>> +  /* GCC split stack support.  */
>> +  void *__private_ss;
>
> Similarily, no need for namespace control.
>
> Andreas.
>

I've named both consistently with x86.  While it doesn't matter for 
__glibc_reserved1, it'd be better to keep __private_ss called the same 
on all platforms (even if we never use it in glibc itself).

Marcin Kościelnicki
Marcin Kościelnicki - Jan. 13, 2016, 2:12 p.m.
> Yes, the initial patch was sent prior the release dates. The only bit I
 > would some comments is why adding '__glibc_reserved1' fields (alignment
 > maybe?).

That's because __private_ss offset from thread pointer is hardcoded in 
gcc, and needs to be constant regardless of glibc configuration (it 
works out to 0x20 for 31-bit, 0x38 for 64-bit) - the same thing is done 
in i386 and x86_64 tcbhead.

 >
 > On 12-01-2016 10:21, Stefan Liebler wrote:
 > > This is okay for s390.
 > >
 > > Adhemerval, is this okay for 2.23?
 > >
 > > It is needed to enable split-stack support on s390 (See gcc bug 
#68191).
 > >
 > > Bye Stefan
 > >

I don't have push access, how do I get the patch applied?

Marcin Kościelnicki
Carlos O'Donell - Jan. 13, 2016, 2:50 p.m.
On 01/13/2016 02:52 AM, Marcin Kościelnicki wrote:
> On 13/01/16 03:41, Carlos O'Donell wrote:
>> On 01/02/2016 11:34 AM, Marcin Kościelnicki wrote:
>>> Preparation for gcc -fsplit-stack support (gcc bug #68191).  The
>>> new field is basically identical to the one on x86.  Its TCB
>>> offset needs to be constant, as it'll be hardcoded in gcc.
>>> 
>>> * sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
>> 
>> What happens if you run newly compiled code with split-stack
>> support on a glibc that doesn't include this space allocated in
>> tcbhead_t? You get a write beyond the tcbhead_t into some other
>> data? Depending on the thread memory layout that could be a guard
>> page or static TLS data?
>> 
>> This is the same problem we saw in POWER when adding a field in
>> tcbhead_t for fast-architecture access.
>> 
>> The only way to do this compatibly is to add a versioned symbol
>> that the compiler references to in order to prevent new binaries
>> from running on old glibc and crashing or worse silently corrupting
>> data.
>> 
>> To reiterate: How do you plan to handle compatibility for this new
>> feature?
>> 
>> If you did an analysis of nptl/allocatestack.c and showed that
>> there were still alignment bytes left, that might be one way to
>> work around this without needing a versioned reference.
>> 
> 
> I have added this field at the end of tcbhead_t, which is part of
> struct pthread, defined here:
> https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/descr.h;h=8e4938deb5311a325d6b67fbc3041037110c5f9e;hb=HEAD#l129
> . The tcbhead_t is stuffed there inside a union with void
> *__padding[24], which is much longer than current tcbhead_t. Thus
> applying this patch to glibc won't even change anything in compiler
> output - there's always been space there, and this patch merely
> reserves it for split-stack use.

Perfect. Sorry I missed that.

> The situation is different for POWER, since it has TLS_DTV_AT_TP - in
> this case, there's no padding for tcbhead_t and no extra expension
> space.

Correct.

Cheers,
Carlos.
Carlos O'Donell - Jan. 13, 2016, 3:44 p.m.
On 01/13/2016 09:12 AM, Marcin Kościelnicki wrote:
>> Yes, the initial patch was sent prior the release dates. The only bit I
>> would some comments is why adding '__glibc_reserved1' fields (alignment
>> maybe?).
> 
> That's because __private_ss offset from thread pointer is hardcoded in gcc, and needs to be constant regardless of glibc configuration (it works out to 0x20 for 31-bit, 0x38 for 64-bit) - the same thing is done in i386 and x86_64 tcbhead.
> 
>>
>> On 12-01-2016 10:21, Stefan Liebler wrote:
>> > This is okay for s390.
>> >
>> > Adhemerval, is this okay for 2.23?
>> >
>> > It is needed to enable split-stack support on s390 (See gcc bug #68191).
>> >
>> > Bye Stefan
>> >
> 
> I don't have push access, how do I get the patch applied?

Stefan Liebler should be able to push it for you.

Cheers,
Carlos.
Stefan Liebler - Jan. 14, 2016, 3:56 p.m.
On 01/13/2016 04:44 PM, Carlos O'Donell wrote:
> On 01/13/2016 09:12 AM, Marcin Kościelnicki wrote:
>>> Yes, the initial patch was sent prior the release dates. The only bit I
>>> would some comments is why adding '__glibc_reserved1' fields (alignment
>>> maybe?).
>>
>> That's because __private_ss offset from thread pointer is hardcoded in gcc, and needs to be constant regardless of glibc configuration (it works out to 0x20 for 31-bit, 0x38 for 64-bit) - the same thing is done in i386 and x86_64 tcbhead.
>>
>>>
>>> On 12-01-2016 10:21, Stefan Liebler wrote:
>>>> This is okay for s390.
>>>>
>>>> Adhemerval, is this okay for 2.23?
>>>>
>>>> It is needed to enable split-stack support on s390 (See gcc bug #68191).
>>>>
>>>> Bye Stefan
>>>>
>>
>> I don't have push access, how do I get the patch applied?
>
> Stefan Liebler should be able to push it for you.
>
> Cheers,
> Carlos.
>


I've applied the patch.
(As information: Marcin has FSF copyright assignment for glibc)

Thanks
Stefan

Patch

diff --git a/ChangeLog b/ChangeLog
index 8c339d2..e39e2ce 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@ 
+2015-01-02  Marcin Kościelnicki  <koriakin@0x04.net>
+
+	* sysdeps/s390/nptl/tls.h: Add __private_ss to struct tcbhead.
+
 2016-01-01  Mike Frysinger  <vapier@gentoo.org>
 
 	[BZ #15421]
diff --git a/sysdeps/s390/nptl/tls.h b/sysdeps/s390/nptl/tls.h
index 28a2119..0e61154 100644
--- a/sysdeps/s390/nptl/tls.h
+++ b/sysdeps/s390/nptl/tls.h
@@ -53,7 +53,11 @@  typedef struct
   int gscope_flag;
 #ifndef __ASSUME_PRIVATE_FUTEX
   int private_futex;
+#else
+  int __glibc_reserved1;
 #endif
+  /* GCC split stack support.  */
+  void *__private_ss;
 } tcbhead_t;
 
 # ifndef __s390x__