Message ID | 2b2cc2fe-2db8-ba60-62e9-d45ef48789eb@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
Series | S390: Fix struct sigaction for 31bit in kernel_sigaction.h. | expand |
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
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! */ unsigned long sa_flags; void (*sa_restorer)(void); sigset_t sa_mask; -#endif }; #define SET_SA_RESTORER(kact, act) \