[PATCH 2.27] RISC-V: fix struct kernel_sigaction to match the kernel version [BZ #23069]

[PATCH 2.27] RISC-V: fix struct kernel_sigaction to match the kernel version [BZ #23069]

[Aurelien Jarno]

The RISC-V kernel doesn't define SA_RESTORER, hence the kernel version
of struct sigaction doesn't have the sa_restorer field. The default
kernel_sigaction.h therefore can't be used.

This patch adds a RISC-V specific version of kernel_sigaction.h to fix
the issue. This fixes for example the libnih testsuite.

Note that this patch is not needed in master as the bug has been fixed
by commit b4a5d26d8835 ("linux: Consolidate sigaction implementation").
---
 ChangeLog                                        | 5 +++++
 sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h | 7 +++++++
 2 files changed, 12 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h

diff --git a/ChangeLog b/ChangeLog
index 536fcf1c4f..f365475a47 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-16  Aurelien Jarno  <aurelien@aurel32.net>
+
+	[BZ #23069]
+	* sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h: New file.
+
 2018-03-29  Florian Weimer  <fweimer@redhat.com>
 
 	* sysdeps/unix/sysv/linux/i386/tst-bz21269.c (do_test): Also
diff --git a/sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h b/sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h
new file mode 100644
index 0000000000..2a62bcc5bc
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h
@@ -0,0 +1,7 @@
+/* This is the sigaction structure from the RISC-V Linux 4.15 kernel.  */
+
+struct kernel_sigaction {
+	__sighandler_t k_sa_handler;
+	unsigned long sa_flags;
+	sigset_t sa_mask;
+};
-- 
2.16.3

Re: [PATCH 2.27] RISC-V: fix struct kernel_sigaction to match the kernel version [BZ #23069]

[DJ Delorie]

Could the #ifdef from b4a5d26d8835's kernel_sigaction.h be backported
instead?  It still wouldn't be a pristine backport (I'm not recommending
backporting *all* of b4a5d26d8835) but at least as branch-specific
'hack' it's closer to what's in master than a riscv-specific header.

Granted, this means a riscv-specific patch *might* affect other targets,
but if that's not an issue, I think something closer to master would be
preferred.

Re: [PATCH 2.27] RISC-V: fix struct kernel_sigaction to match the kernel version [BZ #23069]

[Aurelien Jarno]

On 2018-04-16 17:54, DJ Delorie wrote:
> 
> Could the #ifdef from b4a5d26d8835's kernel_sigaction.h be backported
> instead?  It still wouldn't be a pristine backport (I'm not recommending
> backporting *all* of b4a5d26d8835) but at least as branch-specific
> 'hack' it's closer to what's in master than a riscv-specific header.

We can backport that #ifdef from b4a5d26d8835, but it means we also need
to backport the new kernel_sigaction.h files for m68k, s390, sparc, tile
(if I haven't missed any), plus commit d8abfab7d46a to fix a regression
on s390.

So it seems more risky to me. Also note that this is not an installed
header, but just a compile time header.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH 2.27] RISC-V: fix struct kernel_sigaction to match the kernel version [BZ #23069]

[DJ Delorie]

Aurelien Jarno <aurelien@aurel32.net> writes:
> So it seems more risky to me.

I agree.

> Also note that this is not an installed header, but just a compile
> time header.

So no user-visible ABI change, good.  Ok with me then :-)

Re: [PATCH 2.27] RISC-V: fix struct kernel_sigaction to match the kernel version [BZ #23069]

[Adhemerval Zanella]

On 17/04/2018 12:14, DJ Delorie wrote:
> 
> Aurelien Jarno <aurelien@aurel32.net> writes:
>> So it seems more risky to me.
> 
> I agree.

+1 from me.

> 
>> Also note that this is not an installed header, but just a compile
>> time header.
> 
> So no user-visible ABI change, good.  Ok with me then :-)
> 

Re: [PATCH 2.27] RISC-V: fix struct kernel_sigaction to match the kernel version [BZ #23069]

[Joseph Myers]

On Mon, 16 Apr 2018, Aurelien Jarno wrote:

> The RISC-V kernel doesn't define SA_RESTORER, hence the kernel version
> of struct sigaction doesn't have the sa_restorer field. The default
> kernel_sigaction.h therefore can't be used.
> 
> This patch adds a RISC-V specific version of kernel_sigaction.h to fix
> the issue. This fixes for example the libnih testsuite.

I think there ought to be a relevant test added to the glibc testsuite (on 
master and backported) if there isn't one already, unless it's hard to 
test this issue for some reason.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 2.27] RISC-V: fix struct kernel_sigaction to match the kernel version [BZ #23069]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

On Mon, Apr 16, 2018 at 10:41:32PM +0200, Aurelien Jarno wrote:
> The RISC-V kernel doesn't define SA_RESTORER, hence the kernel version
> of struct sigaction doesn't have the sa_restorer field. The default
> kernel_sigaction.h therefore can't be used.
> 
> This patch adds a RISC-V specific version of kernel_sigaction.h to fix
> the issue. This fixes for example the libnih testsuite.
> 
> Note that this patch is not needed in master as the bug has been fixed
> by commit b4a5d26d8835 ("linux: Consolidate sigaction implementation").
> ---
>  ChangeLog                                        | 5 +++++
>  sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h | 7 +++++++
>  2 files changed, 12 insertions(+)
>  create mode 100644 sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h
> 
> diff --git a/ChangeLog b/ChangeLog
> index 536fcf1c4f..f365475a47 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2018-04-16  Aurelien Jarno  <aurelien@aurel32.net>
> +
> +	[BZ #23069]
> +	* sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h: New file.
> +
>  2018-03-29  Florian Weimer  <fweimer@redhat.com>
>  
>  	* sysdeps/unix/sysv/linux/i386/tst-bz21269.c (do_test): Also
> diff --git a/sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h b/sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h
> new file mode 100644
> index 0000000000..2a62bcc5bc
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/riscv/kernel_sigaction.h
> @@ -0,0 +1,7 @@
> +/* This is the sigaction structure from the RISC-V Linux 4.15 kernel.  */
> +
> +struct kernel_sigaction {
> +	__sighandler_t k_sa_handler;
> +	unsigned long sa_flags;
> +	sigset_t sa_mask;
> +};

I think this is OK given that b4a5d26d8835 cannot be easily backported to 2.27.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH] Add tst-sigaction.c to test BZ #23069

[Aurelien Jarno]

This simple test uses sigaction to define a signal handler. It then
uses sigaction again to fetch the information about the same signal
handler, and check that they are consistent. This is enough to detect
mismatches between struct kernel_sigaction and the kernel version of
struct sigaction, like in BZ #23069.

Changelog:
       * signal/tst-sigaction.c: New file to test BZ #23069.
       * signal/Makefile (tests): Fix indentation. Add tst-sigaction.
---
 ChangeLog              |  5 +++++
 signal/Makefile        |  2 +-
 signal/tst-sigaction.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)
 create mode 100644 signal/tst-sigaction.c

diff --git a/ChangeLog b/ChangeLog
index 6b17507d53..31b8bb6e07 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-22  Aurelien Jarno  <aurelien@aurel32.net>
+
+	* signal/tst-sigaction.c: New file to test BZ #23069.
+	* signal/Makefile (tests): Fix indentation. Add tst-sigaction.
+
 2018-04-20  Joseph Myers  <joseph@codesourcery.com>
 
 	* sysdeps/unix/sysv/linux/alpha/bits/termios.h [__USE_MISC]
diff --git a/signal/Makefile b/signal/Makefile
index a9b99a20be..aa63434f47 100644
--- a/signal/Makefile
+++ b/signal/Makefile
@@ -46,7 +46,7 @@ routines	:= signal raise killpg \
 		   sighold sigrelse sigignore sigset
 
 tests		:= tst-signal tst-sigset tst-sigsimple tst-raise tst-sigset2 \
-  tst-sigwait-eintr \
+		   tst-sigwait-eintr tst-sigaction \
 
 include ../Rules
 
diff --git a/signal/tst-sigaction.c b/signal/tst-sigaction.c
new file mode 100644
index 0000000000..d988df1db8
--- /dev/null
+++ b/signal/tst-sigaction.c
@@ -0,0 +1,55 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <signal.h>
+#include <unistd.h>
+
+#include <support/check.h>
+
+static void
+my_sig_handler (int signum)
+{
+}
+
+static int
+do_test (void)
+{
+  /* Define a simple signal handler */
+  struct sigaction act;
+  act.sa_handler = my_sig_handler;
+  act.sa_flags = 0;
+  sigemptyset (&act.sa_mask);
+
+  /* Set it as SIGUSR1 signal handler */
+  TEST_VERIFY_EXIT (sigaction (SIGUSR1, &act, NULL) == 0);
+
+  /* Get SIGUSR1 signal handler */
+  TEST_VERIFY_EXIT (sigaction (SIGUSR1, NULL, &act) == 0);
+
+  /* Check it is consistent with the defined one */
+  TEST_VERIFY (act.sa_handler == my_sig_handler);
+  TEST_VERIFY (!(act.sa_flags & SA_RESETHAND));
+
+  for (int i = 1; i < _NSIG; i++)
+    {
+      TEST_VERIFY (!sigismember (&act.sa_mask, i));
+    }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.16.3

Re: [PATCH] Add tst-sigaction.c to test BZ #23069

[Adhemerval Zanella]

On 22/04/2018 08:11, Aurelien Jarno wrote:
> This simple test uses sigaction to define a signal handler. It then
> uses sigaction again to fetch the information about the same signal
> handler, and check that they are consistent. This is enough to detect
> mismatches between struct kernel_sigaction and the kernel version of
> struct sigaction, like in BZ #23069.
> 
> Changelog:
>        * signal/tst-sigaction.c: New file to test BZ #23069.
>        * signal/Makefile (tests): Fix indentation. Add tst-sigaction.

LGTM with just one comment below.

> ---
>  ChangeLog              |  5 +++++
>  signal/Makefile        |  2 +-
>  signal/tst-sigaction.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 1 deletion(-)
>  create mode 100644 signal/tst-sigaction.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index 6b17507d53..31b8bb6e07 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2018-04-22  Aurelien Jarno  <aurelien@aurel32.net>
> +
> +	* signal/tst-sigaction.c: New file to test BZ #23069.
> +	* signal/Makefile (tests): Fix indentation. Add tst-sigaction.
> +
>  2018-04-20  Joseph Myers  <joseph@codesourcery.com>
>  
>  	* sysdeps/unix/sysv/linux/alpha/bits/termios.h [__USE_MISC]
> diff --git a/signal/Makefile b/signal/Makefile
> index a9b99a20be..aa63434f47 100644
> --- a/signal/Makefile
> +++ b/signal/Makefile
> @@ -46,7 +46,7 @@ routines	:= signal raise killpg \
>  		   sighold sigrelse sigignore sigset
>  
>  tests		:= tst-signal tst-sigset tst-sigsimple tst-raise tst-sigset2 \
> -  tst-sigwait-eintr \
> +		   tst-sigwait-eintr tst-sigaction \
>  
>  include ../Rules
>  
> diff --git a/signal/tst-sigaction.c b/signal/tst-sigaction.c
> new file mode 100644
> index 0000000000..d988df1db8
> --- /dev/null
> +++ b/signal/tst-sigaction.c
> @@ -0,0 +1,55 @@

Need one line file description.

> +/* Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <signal.h>
> +#include <unistd.h>
> +
> +#include <support/check.h>
> +
> +static void
> +my_sig_handler (int signum)
> +{
> +}
> +
> +static int
> +do_test (void)
> +{
> +  /* Define a simple signal handler */
> +  struct sigaction act;
> +  act.sa_handler = my_sig_handler;
> +  act.sa_flags = 0;
> +  sigemptyset (&act.sa_mask);
> +
> +  /* Set it as SIGUSR1 signal handler */
> +  TEST_VERIFY_EXIT (sigaction (SIGUSR1, &act, NULL) == 0);
> +
> +  /* Get SIGUSR1 signal handler */
> +  TEST_VERIFY_EXIT (sigaction (SIGUSR1, NULL, &act) == 0);
> +
> +  /* Check it is consistent with the defined one */
> +  TEST_VERIFY (act.sa_handler == my_sig_handler);
> +  TEST_VERIFY (!(act.sa_flags & SA_RESETHAND));
> +
> +  for (int i = 1; i < _NSIG; i++)
> +    {
> +      TEST_VERIFY (!sigismember (&act.sa_mask, i));
> +    }
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
>