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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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)             \
>