Bug 16713 - [s390x] Member "sa_flags" does not have the correct type

  Hi,

[Sorry for the duplicate Stefan, I got the list name wrong on the CC.]

To fix the following bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=16713
There was this simple change:


But means glibc and the kernel don't agree anymore on the structure
layout of struct sigaction. The kernel is still using unsigned long
sa_flags on s390. I guess this still works out because all current
SA_... flag values fit in an int. But it does mean the kernel cannot
expand the flag field values anymore in the future.

Because valgrind follows the kernel definition the above change causes
valgrind memcheck to complain now whenever only the sa_flags value (as
seen by user space) is assigned. For example in nptl-init.c:

  struct sigaction sa;
  sa.sa_sigaction = sigcancel_handler;
  sa.sa_flags = SA_SIGINFO;
  __sigemptyset (&sa.sa_mask);

  (void) __libc_sigaction (SIGCANCEL, &sa, NULL);

valgrind will now warn:

 Syscall param rt_sigaction(act->sa_flags) points to uninitialised byte(s)
    at 0x42EECC8: __libc_sigaction (sigaction.c:42)
    by 0x42E2253: __pthread_initialize_minimal (nptl-init.c:381)
    by 0x42E15EF: ??? (in /usr/lib64/libpthread-2.19.90.so)

Because the '__glibc_reserved0' part of sa_flags as the kernel will see
them is indeed undefined.

We can easily change valgrind to follow the layout as now used by
glibc/user space on s390 instead of the kernel layout and just ignore
the __glibc_reserved0 field. But I wanted to first make sure this
change/difference between glibc/kernel was intended.

Maybe glibc wants to pass the struct as intended by the kernel and needs
to create a "bridge" struct that properly initializes the kernel's view
of sa_flags?

Thanks,

Mark
  

On Tuesday, September 09 2014, Mark Wielaard wrote:

> Maybe glibc wants to pass the struct as intended by the kernel and needs
> to create a "bridge" struct that properly initializes the kernel's view
> of sa_flags?

BTW, as discussed with Mark, a similar approach is done in GDB already.

However, I think the question is: does glibc need to have both
declarations?  IIUC, this difference was introduced because of POSIX
conformance, but if it does more harm than good...

Anyway, I'm really out of my scope here.  Just my 0.0001 cent.
  

On Tue, Sep 09, 2014 at 08:49:22PM -0400, Sergio Durigan Junior wrote:
> On Tuesday, September 09 2014, Mark Wielaard wrote:
> 
> > Maybe glibc wants to pass the struct as intended by the kernel and needs
> > to create a "bridge" struct that properly initializes the kernel's view
> > of sa_flags?
> 
> BTW, as discussed with Mark, a similar approach is done in GDB already.
> 
> However, I think the question is: does glibc need to have both
> declarations?  IIUC, this difference was introduced because of POSIX
> conformance, but if it does more harm than good...
> 
> Anyway, I'm really out of my scope here.  Just my 0.0001 cent.

glibc needs to have a POSIX conforming public definition of struct
sigaction. If there's any risk of the kernel interpreting the junk in
the upper bits due to the kernel's incorrect definition of the
structure, then glibc needs to make a copy to pass to the kernel, with
the unused bits filled with zeros in the copy.

Rich
  

Hi,

yes the kernel and glibc struct differs on s390. This is intended in 
order to get POSIX conform. I got kernel agreement before i changed the 
struct. The kernel has no problem with the __glibc_reserved0 variable 
and its random content.

Bye
Stefan

On 09/10/2014 01:31 AM, Mark Wielaard wrote:
> Hi,
>
> [Sorry for the duplicate Stefan, I got the list name wrong on the CC.]
>
> To fix the following bug:
> https://sourceware.org/bugzilla/show_bug.cgi?id=16713
> There was this simple change:
>
> --- a/sysdeps/unix/sysv/linux/s390/bits/sigaction.h
> +++ b/sysdeps/unix/sysv/linux/s390/bits/sigaction.h
> @@ -43,7 +43,8 @@ struct sigaction
>   #endif
>
>       /* Special flags.  */
> -    unsigned long int sa_flags;
> +    int __glibc_reserved0;
> +    int sa_flags;
>
>       /* Restore handler.  */
>       void (*sa_restorer) (void);
>
> But means glibc and the kernel don't agree anymore on the structure
> layout of struct sigaction. The kernel is still using unsigned long
> sa_flags on s390. I guess this still works out because all current
> SA_... flag values fit in an int. But it does mean the kernel cannot
> expand the flag field values anymore in the future.
>
> Because valgrind follows the kernel definition the above change causes
> valgrind memcheck to complain now whenever only the sa_flags value (as
> seen by user space) is assigned. For example in nptl-init.c:
>
>    struct sigaction sa;
>    sa.sa_sigaction = sigcancel_handler;
>    sa.sa_flags = SA_SIGINFO;
>    __sigemptyset (&sa.sa_mask);
>
>    (void) __libc_sigaction (SIGCANCEL, &sa, NULL);
>
> valgrind will now warn:
>
>   Syscall param rt_sigaction(act->sa_flags) points to uninitialised byte(s)
>      at 0x42EECC8: __libc_sigaction (sigaction.c:42)
>      by 0x42E2253: __pthread_initialize_minimal (nptl-init.c:381)
>      by 0x42E15EF: ??? (in /usr/lib64/libpthread-2.19.90.so)
>
> Because the '__glibc_reserved0' part of sa_flags as the kernel will see
> them is indeed undefined.
>
> We can easily change valgrind to follow the layout as now used by
> glibc/user space on s390 instead of the kernel layout and just ignore
> the __glibc_reserved0 field. But I wanted to first make sure this
> change/difference between glibc/kernel was intended.
>
> Maybe glibc wants to pass the struct as intended by the kernel and needs
> to create a "bridge" struct that properly initializes the kernel's view
> of sa_flags?
>
> Thanks,
>
> Mark
>
>
  

On 09/10/2014 09:18 AM, Stefan Liebler wrote:

> yes the kernel and glibc struct differs on s390. This is intended in
> order to get POSIX conform. I got kernel agreement before i changed the
> struct. The kernel has no problem with the __glibc_reserved0 variable
> and its random content.

But valgrind will need a new suppression for this, I think.
  

On Wed, 2014-09-10 at 09:18 +0200, Stefan Liebler wrote:
> yes the kernel and glibc struct differs on s390. This is intended in 
> order to get POSIX conform. I got kernel agreement before i changed the 
> struct. The kernel has no problem with the __glibc_reserved0 variable 
> and its random content.

Thanks. I'll change valgrind accordingly then on s390 to ignore those
undefined bytes in the sigaction struct:
https://bugs.kde.org/show_bug.cgi?id=338974

Cheers,

Mark
  

On Wed, 10 Sep 2014, Mark Wielaard wrote:

> But means glibc and the kernel don't agree anymore on the structure
> layout of struct sigaction. The kernel is still using unsigned long
> sa_flags on s390. I guess this still works out because all current
> SA_... flag values fit in an int. But it does mean the kernel cannot
> expand the flag field values anymore in the future.

Given that there are platforms where the flags field is 32-bit in the 
kernel, and that these bits seem unlikely to be architecture-specific, 
there wasn't really scope for expanding these values beyond 32-bit anyway.
  

On Tuesday, September 09 2014, Rich Felker wrote:

> glibc needs to have a POSIX conforming public definition of struct
> sigaction. If there's any risk of the kernel interpreting the junk in
> the upper bits due to the kernel's incorrect definition of the
> structure, then glibc needs to make a copy to pass to the kernel, with
> the unused bits filled with zeros in the copy.

Thanks for the explanation.  Then yeah, the "bridge struct" idea seems
like the best one.