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

[Stefan Liebler]

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