Add __private_ss to s390 struct tcbhead.

  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(+)
  

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
  

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__
>
  

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__
>>
>
  

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__
>
  

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
  

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.
  

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
  

> 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
  

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.
  

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.
  

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