S390: Fix struct sigaction for 31bit in kernel_sigaction.h.

  Hi,

the recent commit b4a5d26d8835d972995f0a0a2f805a8845bafa0b
"linux: Consolidate sigaction implementation" changed the definition
of struct sigaction for s390 (31bit). Unfortunately the order of the
fields were wrong.

This leads to blocking testcases e.g. nptl/tst-sem11. A thread which 
blocks due to sem_wait() is cancelled via pthread_cancel() and the 
signal-handler sigcancel_handler (see <glibc-src>/nptl/nptl-init.c is 
called. But it just returns as the siginfo_t argument is not setup by 
the kernel. Then the main-thread is blocking due to pthread_join().

The flag SA_SIGINFO is set in sa_flags in struct sigaction and is copied 
to the "kernel_sigaction.h" struct by the sigaction() call, but due to 
the wrong ordering of the struct fields, the kernel does not recognize it.

This patch is fixing the definition of s390-kernel_sigaction.h struct 
for 31bit.

Okay to commit?

Bye
Stefan

ChangeLog:

	* sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
	(struct kernel_sigaction): Use the same definition
	on 31bit as is used on 64bit.
  

On 11/04/2018 12:22, Stefan Liebler wrote:
> Hi,
> 
> the recent commit b4a5d26d8835d972995f0a0a2f805a8845bafa0b
> "linux: Consolidate sigaction implementation" changed the definition
> of struct sigaction for s390 (31bit). Unfortunately the order of the
> fields were wrong.
> 
> This leads to blocking testcases e.g. nptl/tst-sem11. A thread which blocks due to sem_wait() is cancelled via pthread_cancel() and the signal-handler sigcancel_handler (see <glibc-src>/nptl/nptl-init.c is called. But it just returns as the siginfo_t argument is not setup by the kernel. Then the main-thread is blocking due to pthread_join().
> 
> The flag SA_SIGINFO is set in sa_flags in struct sigaction and is copied to the "kernel_sigaction.h" struct by the sigaction() call, but due to the wrong ordering of the struct fields, the kernel does not recognize it.
> 
> This patch is fixing the definition of s390-kernel_sigaction.h struct for 31bit.
> 
> Okay to commit?

Thanks for checking on it (I wish the LinuxONE access account wouldn't have
a 3 month expiration limit).  LGTM, only the comment sounds a bit confusing.

> 
> Bye
> Stefan
> 
> ChangeLog:
> 
>     * sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
>     (struct kernel_sigaction): Use the same definition
>     on 31bit as is used on 64bit.
> 
> 20180411_s390_kernel_sigaction.patch
> 
> 
> commit 404f5a0eac37d4c85e027c7b7fc64cb3ecbfc894
> Author: Stefan Liebler <stli@linux.vnet.ibm.com>
> Date:   Wed Apr 11 16:57:48 2018 +0200
> 
>     S390: Fix struct sigaction for 31bit in kernel_sigaction.h.
>     
>     The recent commit b4a5d26d8835d972995f0a0a2f805a8845bafa0b
>     "linux: Consolidate sigaction implementation" changed the definition
>     of struct sigaction for s390 (31bit). Unfortunately the order of the
>     fields were wrong.
>     
>     This leads to blocking testcases e.g. nptl/tst-sem11.
>     A thread which blocks due to sem_wait() is cancelled via pthread_cancel()
>     and the signal-handler sigcancel_handler (see <glibc-src>/nptl/nptl-init.c
>     is called.
>     But it just returns as the siginfo_t argument is not setup by the kernel.
>     Then the main-thread is blocking due to pthread_join().
>     
>     The flag SA_SIGINFO is set in sa_flags in struct sigaction and
>     is copied to the "kernel_sigaction.h" struct by the sigaction() call,
>     but due to the wrong ordering of the struct fields,
>     the kernel does not recognize it.
> 
> diff --git a/sysdeps/unix/sysv/linux/s390/kernel_sigaction.h b/sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
> index a8beaf7347..28a1aa0f37 100644
> --- a/sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
> +++ b/sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
> @@ -11,15 +11,30 @@ struct kernel_sigaction
>      void (*_sa_sigaction)(int, siginfo_t *, void *);
>    } _u;
>  #define k_sa_handler _u._sa_handler
> -#ifndef __s390x__
> -  sigset_t sa_mask;
> -  unsigned long sa_flags;
> -  void (*sa_restorer)(void);
> -#else
> +  /* The rt_sigaction-syscall (which is currently used in glibc)
> +     expects this struct on 31bit (real 31bit-kernel or compat-mode) and 64bit!
> +     See <kernel-src>/include/linux/signal_types.h: struct sigaction
> +     or <kernel-src>/include/linux/compat.h: struct compat_sigaction.
> +
> +     The sigaction-syscall (which is currently not used in glibc and was never
> +     used on s390x 64bit) expects the kernel struct old_sigaction
> +     and struct compat_old_sigaction. There the order of the fields is:
> +     -_sa_handler / _sa_sigaction
> +     -sa_mask
> +     -sa_flags
> +     -sa_restorer
> +     See the same kernel-headers as mentioned above.
> +
> +     The definition of struct sigaction in
> +     <kernel-src>/arch/s390/include/uapi/asm/signal.h
> +     (only used for kernel-uapi)
> +     is currently using the struct-definition for rt_sigaction-syscall on 64bit
> +     and the struct-definition for sigaction-syscall on 31bit.
> +     Thus we can't simply copy this definition here.
> +     Note: This kernel-uapi-defintion will also be fixed!  */

I would use just:

/* The 'struct sigaction' definition in s390 kernel header
   arch/s390/include/uapi/asm/signal.h is used for __NR_rt_sigaction
   on 64 bits and for __NR_sigaction for 31 bits.

   The expected layout for __NR_rt_sigaction for 31 bits is either
   'struct sigaction' from include/linux/signal_types.h or
   'struct compat_sigaction' from include/linux/compat.h.

   So for __NR_rt_sigaction we can use the same layout for both s390x
   and s390.  */

>    unsigned long sa_flags;
>    void (*sa_restorer)(void);
>    sigset_t sa_mask;
> -#endif
>  };
>  
>  #define SET_SA_RESTORER(kact, act)             \
>
  

On 04/11/2018 06:56 PM, Adhemerval Zanella wrote:
> 
> 
> On 11/04/2018 12:22, Stefan Liebler wrote:
>> Hi,
>>
>> the recent commit b4a5d26d8835d972995f0a0a2f805a8845bafa0b
>> "linux: Consolidate sigaction implementation" changed the definition
>> of struct sigaction for s390 (31bit). Unfortunately the order of the
>> fields were wrong.
>>
>> This leads to blocking testcases e.g. nptl/tst-sem11. A thread which blocks due to sem_wait() is cancelled via pthread_cancel() and the signal-handler sigcancel_handler (see <glibc-src>/nptl/nptl-init.c is called. But it just returns as the siginfo_t argument is not setup by the kernel. Then the main-thread is blocking due to pthread_join().
>>
>> The flag SA_SIGINFO is set in sa_flags in struct sigaction and is copied to the "kernel_sigaction.h" struct by the sigaction() call, but due to the wrong ordering of the struct fields, the kernel does not recognize it.
>>
>> This patch is fixing the definition of s390-kernel_sigaction.h struct for 31bit.
>>
>> Okay to commit?
> 
> Thanks for checking on it (I wish the LinuxONE access account wouldn't have
> a 3 month expiration limit).  LGTM, only the comment sounds a bit confusing.
> 
>>
>> Bye
>> Stefan
>>
>> ChangeLog:
>>
>>      * sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
>>      (struct kernel_sigaction): Use the same definition
>>      on 31bit as is used on 64bit.
>>
>> 20180411_s390_kernel_sigaction.patch
>>
>>
>> commit 404f5a0eac37d4c85e027c7b7fc64cb3ecbfc894
>> Author: Stefan Liebler <stli@linux.vnet.ibm.com>
>> Date:   Wed Apr 11 16:57:48 2018 +0200
>>
>>      S390: Fix struct sigaction for 31bit in kernel_sigaction.h.
>>      
>>      The recent commit b4a5d26d8835d972995f0a0a2f805a8845bafa0b
>>      "linux: Consolidate sigaction implementation" changed the definition
>>      of struct sigaction for s390 (31bit). Unfortunately the order of the
>>      fields were wrong.
>>      
>>      This leads to blocking testcases e.g. nptl/tst-sem11.
>>      A thread which blocks due to sem_wait() is cancelled via pthread_cancel()
>>      and the signal-handler sigcancel_handler (see <glibc-src>/nptl/nptl-init.c
>>      is called.
>>      But it just returns as the siginfo_t argument is not setup by the kernel.
>>      Then the main-thread is blocking due to pthread_join().
>>      
>>      The flag SA_SIGINFO is set in sa_flags in struct sigaction and
>>      is copied to the "kernel_sigaction.h" struct by the sigaction() call,
>>      but due to the wrong ordering of the struct fields,
>>      the kernel does not recognize it.
>>
>> diff --git a/sysdeps/unix/sysv/linux/s390/kernel_sigaction.h b/sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
>> index a8beaf7347..28a1aa0f37 100644
>> --- a/sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
>> +++ b/sysdeps/unix/sysv/linux/s390/kernel_sigaction.h
>> @@ -11,15 +11,30 @@ struct kernel_sigaction
>>       void (*_sa_sigaction)(int, siginfo_t *, void *);
>>     } _u;
>>   #define k_sa_handler _u._sa_handler
>> -#ifndef __s390x__
>> -  sigset_t sa_mask;
>> -  unsigned long sa_flags;
>> -  void (*sa_restorer)(void);
>> -#else
>> +  /* The rt_sigaction-syscall (which is currently used in glibc)
>> +     expects this struct on 31bit (real 31bit-kernel or compat-mode) and 64bit!
>> +     See <kernel-src>/include/linux/signal_types.h: struct sigaction
>> +     or <kernel-src>/include/linux/compat.h: struct compat_sigaction.
>> +
>> +     The sigaction-syscall (which is currently not used in glibc and was never
>> +     used on s390x 64bit) expects the kernel struct old_sigaction
>> +     and struct compat_old_sigaction. There the order of the fields is:
>> +     -_sa_handler / _sa_sigaction
>> +     -sa_mask
>> +     -sa_flags
>> +     -sa_restorer
>> +     See the same kernel-headers as mentioned above.
>> +
>> +     The definition of struct sigaction in
>> +     <kernel-src>/arch/s390/include/uapi/asm/signal.h
>> +     (only used for kernel-uapi)
>> +     is currently using the struct-definition for rt_sigaction-syscall on 64bit
>> +     and the struct-definition for sigaction-syscall on 31bit.
>> +     Thus we can't simply copy this definition here.
>> +     Note: This kernel-uapi-defintion will also be fixed!  */
> 
> I would use just:
> 
> /* The 'struct sigaction' definition in s390 kernel header
>     arch/s390/include/uapi/asm/signal.h is used for __NR_rt_sigaction
>     on 64 bits and for __NR_sigaction for 31 bits.
> 
>     The expected layout for __NR_rt_sigaction for 31 bits is either
>     'struct sigaction' from include/linux/signal_types.h or
>     'struct compat_sigaction' from include/linux/compat.h.
> 
>     So for __NR_rt_sigaction we can use the same layout for both s390x
>     and s390.  */
> 
>>     unsigned long sa_flags;
>>     void (*sa_restorer)(void);
>>     sigset_t sa_mask;
>> -#endif
>>   };
>>   
>>   #define SET_SA_RESTORER(kact, act)             \
>>
> 

okay. I've committed this patch with your proposed comment.

Thanks.
Stefan