[PATCH 0/4] linux-user: fix use of SIGRTMIN

[PATCH 0/4] linux-user: fix use of SIGRTMIN

[Laurent Vivier]

This series fixes the problem of the first real-time signals already
in use by the glibc that are not available for the target glibc.

Instead of reverting the first and last real-time signals we rely on
the value provided by the glibc (SIGRTMIN) to know the first available
signal and we map all the signals from this value to SIGRTMAX on top
of TARGET_SIGRTMIN. So the consequence is we have less available signals
in the target (generally 2) but all seems fine as at least 30 signals are
still available.

This has been tested with Go (golang 1.10.1 linux/arm64, bionic) on x86_64
fedora 31. We can avoid the failure in this case allowing the unsupported
signals when we don't provide the "act" parameters to sigaction, only the
"oldact" one. I have also run the LTP suite with several target and debian
based distros.

Laurent Vivier (4):
  linux-user: add missing TARGET_SIGRTMIN for hppa
  linux-user: cleanup signal.c
  linux-user: fix TARGET_NSIG and _NSIG uses
  linux-user: fix use of SIGRTMIN

 linux-user/hppa/target_signal.h |   1 +
 linux-user/signal.c             | 110 +++++++++++++++++++++++---------
 linux-user/trace-events         |   3 +
 3 files changed, 85 insertions(+), 29 deletions(-)

-- 
2.24.1

[PATCH 1/4] linux-user: add missing TARGET_SIGRTMIN for hppa

[Laurent Vivier]

This signal is defined for all other targets and we will need it later

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/hppa/target_signal.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/hppa/target_signal.h b/linux-user/hppa/target_signal.h
index ba159ff8d006..c2a0102ed73d 100644
--- a/linux-user/hppa/target_signal.h
+++ b/linux-user/hppa/target_signal.h
@@ -34,6 +34,7 @@
 #define TARGET_SIGURG          29
 #define TARGET_SIGXFSZ         30
 #define TARGET_SIGSYS          31
+#define TARGET_SIGRTMIN        32
 
 #define TARGET_SIG_BLOCK       0
 #define TARGET_SIG_UNBLOCK     1
-- 
2.24.1

[PATCH 2/4] linux-user: cleanup signal.c

[Laurent Vivier]

No functionnal changes. Prepare the field for future fixes.

Remove memset(.., 0, ...) that is useless on a static array

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/signal.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5ca6d62b15d3..f42a2e1a82a5 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -66,12 +66,6 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
     [SIGPWR] = TARGET_SIGPWR,
     [SIGSYS] = TARGET_SIGSYS,
     /* next signals stay the same */
-    /* Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
-       host libpthread signals.  This assumes no one actually uses SIGRTMAX :-/
-       To fix this properly we need to do manual signal delivery multiplexed
-       over a single host signal.  */
-    [__SIGRTMIN] = __SIGRTMAX,
-    [__SIGRTMAX] = __SIGRTMIN,
 };
 static uint8_t target_to_host_signal_table[_NSIG];
 
@@ -480,13 +474,18 @@ static int core_dump_signal(int sig)
     }
 }
 
-void signal_init(void)
+static void signal_table_init(void)
 {
-    TaskState *ts = (TaskState *)thread_cpu->opaque;
-    struct sigaction act;
-    struct sigaction oact;
     int i, j;
-    int host_sig;
+
+    /*
+     * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
+     * host libpthread signals.  This assumes no one actually uses SIGRTMAX :-/
+     * To fix this properly we need to do manual signal delivery multiplexed
+     * over a single host signal.
+     */
+    host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX;
+    host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
 
     /* generate signal conversion tables */
     for(i = 1; i < _NSIG; i++) {
@@ -497,14 +496,22 @@ void signal_init(void)
         j = host_to_target_signal_table[i];
         target_to_host_signal_table[j] = i;
     }
+}
+
+void signal_init(void)
+{
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+    struct sigaction act;
+    struct sigaction oact;
+    int i;
+    int host_sig;
+
+    /* initialize signal conversion tables */
+    signal_table_init();
 
     /* Set the signal mask from the host mask. */
     sigprocmask(0, 0, &ts->signal_mask);
 
-    /* set all host signal handlers. ALL signals are blocked during
-       the handlers to serialize them. */
-    memset(sigact_table, 0, sizeof(sigact_table));
-
     sigfillset(&act.sa_mask);
     act.sa_flags = SA_SIGINFO;
     act.sa_sigaction = host_signal_handler;
-- 
2.24.1

[PATCH 3/4] linux-user: fix TARGET_NSIG and _NSIG uses

[Laurent Vivier]

Valid signal numbers are between 1 (SIGHUP) and SIGRTMAX.

System includes define _NSIG to SIGRTMAX + 1, but
QEMU (like kernel) defines TARGET_NSIG to TARGET_SIGRTMAX.

Fix all the checks involving the signal range.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/signal.c | 51 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index f42a2e1a82a5..3491f0a7ecb1 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -30,6 +30,15 @@ static struct target_sigaction sigact_table[TARGET_NSIG];
 static void host_signal_handler(int host_signum, siginfo_t *info,
                                 void *puc);
 
+
+/*
+ * System includes define _NSIG as SIGRTMAX + 1,
+ * but qemu (like the kernel) defines TARGET_NSIG as TARGET_SIGRTMAX
+ * and the first signal is SIGHUP defined as 1
+ * Signal number 0 is reserved for use as kill(pid, 0), to test whether
+ * a process exists without sending it a signal.
+ */
+QEMU_BUILD_BUG_ON(__SIGRTMAX + 1 != _NSIG);
 static uint8_t host_to_target_signal_table[_NSIG] = {
     [SIGHUP] = TARGET_SIGHUP,
     [SIGINT] = TARGET_SIGINT,
@@ -67,19 +76,24 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
     [SIGSYS] = TARGET_SIGSYS,
     /* next signals stay the same */
 };
-static uint8_t target_to_host_signal_table[_NSIG];
 
+static uint8_t target_to_host_signal_table[TARGET_NSIG + 1];
+
+/* valid sig is between 1 and _NSIG - 1 */
 int host_to_target_signal(int sig)
 {
-    if (sig < 0 || sig >= _NSIG)
+    if (sig < 1 || sig >= _NSIG) {
         return sig;
+    }
     return host_to_target_signal_table[sig];
 }
 
+/* valid sig is between 1 and TARGET_NSIG */
 int target_to_host_signal(int sig)
 {
-    if (sig < 0 || sig >= _NSIG)
+    if (sig < 1 || sig > TARGET_NSIG) {
         return sig;
+    }
     return target_to_host_signal_table[sig];
 }
 
@@ -100,11 +114,15 @@ static inline int target_sigismember(const target_sigset_t *set, int signum)
 void host_to_target_sigset_internal(target_sigset_t *d,
                                     const sigset_t *s)
 {
-    int i;
+    int i, j;
     target_sigemptyset(d);
-    for (i = 1; i <= TARGET_NSIG; i++) {
+    for (i = 1; i < _NSIG; i++) {
+        j = host_to_target_signal(i);
+        if (j < 1 || j > TARGET_NSIG) {
+            continue;
+        }
         if (sigismember(s, i)) {
-            target_sigaddset(d, host_to_target_signal(i));
+            target_sigaddset(d, j);
         }
     }
 }
@@ -122,11 +140,15 @@ void host_to_target_sigset(target_sigset_t *d, const sigset_t *s)
 void target_to_host_sigset_internal(sigset_t *d,
                                     const target_sigset_t *s)
 {
-    int i;
+    int i, j;
     sigemptyset(d);
     for (i = 1; i <= TARGET_NSIG; i++) {
+        j = target_to_host_signal(i);
+        if (j < 1 || j >= _NSIG) {
+            continue;
+        }
         if (target_sigismember(s, i)) {
-            sigaddset(d, target_to_host_signal(i));
+            sigaddset(d, j);
         }
     }
 }
@@ -488,13 +510,14 @@ static void signal_table_init(void)
     host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
 
     /* generate signal conversion tables */
-    for(i = 1; i < _NSIG; i++) {
-        if (host_to_target_signal_table[i] == 0)
+    for (i = 1; i < _NSIG; i++) {
+        if (host_to_target_signal_table[i] == 0) {
             host_to_target_signal_table[i] = i;
-    }
-    for(i = 1; i < _NSIG; i++) {
+        }
         j = host_to_target_signal_table[i];
-        target_to_host_signal_table[j] = i;
+        if (j <= TARGET_NSIG) {
+            target_to_host_signal_table[j] = i;
+        }
     }
 }
 
@@ -517,7 +540,7 @@ void signal_init(void)
     act.sa_sigaction = host_signal_handler;
     for(i = 1; i <= TARGET_NSIG; i++) {
 #ifdef TARGET_GPROF
-        if (i == SIGPROF) {
+        if (i == TARGET_SIGPROF) {
             continue;
         }
 #endif
-- 
2.24.1

[PATCH 4/4] linux-user: fix use of SIGRTMIN

[Laurent Vivier]

Some RT signals can be in use by glibc,
it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32).

So SIGRTMIN cannot be mapped to TARGET_SIGRTMIN.

Instead of swapping only SIGRTMIN and SIGRTMAX, map all the
range [TARGET_SIGRTMIN ... TARGET_SIGRTMAX - X] to
      [__SIGRTMIN + X ... SIGRTMAX ]
(SIGRTMIN is __SIGRTMIN + X).

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/signal.c     | 34 ++++++++++++++++++++++++++++------
 linux-user/trace-events |  3 +++
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 3491f0a7ecb1..c4abacde49a0 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -501,15 +501,20 @@ static void signal_table_init(void)
     int i, j;
 
     /*
-     * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
-     * host libpthread signals.  This assumes no one actually uses SIGRTMAX :-/
-     * To fix this properly we need to do manual signal delivery multiplexed
-     * over a single host signal.
+     * some RT signals can be in use by glibc,
+     * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32)
      */
-    host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX;
-    host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
+    for (i = SIGRTMIN; i <= SIGRTMAX; i++) {
+        j = i - SIGRTMIN + TARGET_SIGRTMIN;
+        if (j <= TARGET_NSIG) {
+            host_to_target_signal_table[i] = j;
+        }
+    }
 
     /* generate signal conversion tables */
+    for (i = 1; i <= TARGET_NSIG; i++) {
+        target_to_host_signal_table[i] = _NSIG; /* poison */
+    }
     for (i = 1; i < _NSIG; i++) {
         if (host_to_target_signal_table[i] == 0) {
             host_to_target_signal_table[i] = i;
@@ -519,6 +524,15 @@ static void signal_table_init(void)
             target_to_host_signal_table[j] = i;
         }
     }
+
+    if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) {
+        for (i = 1, j = 0; i <= TARGET_NSIG; i++) {
+            if (target_to_host_signal_table[i] == _NSIG) {
+                j++;
+            }
+        }
+        trace_signal_table_init(j);
+    }
 }
 
 void signal_init(void)
@@ -817,6 +831,8 @@ int do_sigaction(int sig, const struct target_sigaction *act,
     int host_sig;
     int ret = 0;
 
+    trace_signal_do_sigaction_guest(sig, TARGET_NSIG);
+
     if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig == TARGET_SIGSTOP) {
         return -TARGET_EINVAL;
     }
@@ -847,6 +863,12 @@ int do_sigaction(int sig, const struct target_sigaction *act,
 
         /* we update the host linux signal state */
         host_sig = target_to_host_signal(sig);
+        trace_signal_do_sigaction_host(host_sig, TARGET_NSIG);
+        if (host_sig > SIGRTMAX) {
+            /* we don't have enough host signals to map all target signals */
+            qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d\n", sig);
+            return -TARGET_EINVAL;
+        }
         if (host_sig != SIGSEGV && host_sig != SIGBUS) {
             sigfillset(&act1.sa_mask);
             act1.sa_flags = SA_SIGINFO;
diff --git a/linux-user/trace-events b/linux-user/trace-events
index f6de1b8befc0..eb4b7701c400 100644
--- a/linux-user/trace-events
+++ b/linux-user/trace-events
@@ -1,6 +1,9 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # signal.c
+signal_table_init(int i) "missing signal number: %d"
+signal_do_sigaction_guest(int sig, int max) "target signal %d (MAX %d)"
+signal_do_sigaction_host(int sig, int max) "host signal %d (MAX %d)"
 # */signal.c
 user_setup_frame(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64
 user_setup_rt_frame(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64
-- 
2.24.1

RE: [PATCH 0/4] linux-user: fix use of SIGRTMIN

[Taylor Simpson]

FWIW, this removes the need for the target-specific code for Hexagon in signal.c.

Thanks,
Taylor

PS  Stay tuned for a Hexagon target patch series once this is merged.

> -----Original Message-----
> From: Laurent Vivier <laurent@vivier.eu>
> Sent: Saturday, February 1, 2020 6:28 AM
> To: qemu-devel@nongnu.org
> Cc: Josh Kunz <jkz@google.com>; milos.stojanovic@rt-rk.com; Matus Kysel
> <mkysel@tachyum.com>; Aleksandar Markovic <aleksandar.markovic@rt-
> rk.com>; Marlies Ruck <marlies.ruck@gmail.com>; Laurent Vivier
> <laurent@vivier.eu>; Peter Maydell <peter.maydell@linaro.org>; Taylor
> Simpson <tsimpson@quicinc.com>; Riku Voipio <riku.voipio@iki.fi>
> Subject: [PATCH 0/4] linux-user: fix use of SIGRTMIN
>
> This series fixes the problem of the first real-time signals already in use by
> the glibc that are not available for the target glibc.
>
> Instead of reverting the first and last real-time signals we rely on the value
> provided by the glibc (SIGRTMIN) to know the first available signal and we
> map all the signals from this value to SIGRTMAX on top of
> TARGET_SIGRTMIN. So the consequence is we have less available signals in
> the target (generally 2) but all seems fine as at least 30 signals are still
> available.
>
> This has been tested with Go (golang 1.10.1 linux/arm64, bionic) on x86_64
> fedora 31. We can avoid the failure in this case allowing the unsupported
> signals when we don't provide the "act" parameters to sigaction, only the
> "oldact" one. I have also run the LTP suite with several target and debian
> based distros.
>
> Laurent Vivier (4):
>   linux-user: add missing TARGET_SIGRTMIN for hppa
>   linux-user: cleanup signal.c
>   linux-user: fix TARGET_NSIG and _NSIG uses
>   linux-user: fix use of SIGRTMIN
>
>  linux-user/hppa/target_signal.h |   1 +
>  linux-user/signal.c             | 110 +++++++++++++++++++++++---------
>  linux-user/trace-events         |   3 +
>  3 files changed, 85 insertions(+), 29 deletions(-)
>
> --
> 2.24.1
>

RE: [PATCH 2/4] linux-user: cleanup signal.c

[Taylor Simpson]

> -----Original Message-----
> From: Laurent Vivier <laurent@vivier.eu>
> Sent: Saturday, February 1, 2020 6:28 AM
> To: qemu-devel@nongnu.org
> Cc: Josh Kunz <jkz@google.com>; milos.stojanovic@rt-rk.com; Matus Kysel
> <mkysel@tachyum.com>; Aleksandar Markovic <aleksandar.markovic@rt-
> rk.com>; Marlies Ruck <marlies.ruck@gmail.com>; Laurent Vivier
> <laurent@vivier.eu>; Peter Maydell <peter.maydell@linaro.org>; Taylor
> Simpson <tsimpson@quicinc.com>; Riku Voipio <riku.voipio@iki.fi>
> Subject: [PATCH 2/4] linux-user: cleanup signal.c
>
> -------------------------------------------------------------------------
> CAUTION: This email originated from outside of the organization.
> -------------------------------------------------------------------------
>
> No functionnal changes. Prepare the field for future fixes.


Spelling error

>
> Remove memset(.., 0, ...) that is useless on a static array
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/signal.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c index
> 5ca6d62b15d3..f42a2e1a82a5 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -66,12 +66,6 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
>      [SIGPWR] = TARGET_SIGPWR,
>      [SIGSYS] = TARGET_SIGSYS,
>      /* next signals stay the same */
> -    /* Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
> -       host libpthread signals.  This assumes no one actually uses SIGRTMAX :-/
> -       To fix this properly we need to do manual signal delivery multiplexed
> -       over a single host signal.  */
> -    [__SIGRTMIN] = __SIGRTMAX,
> -    [__SIGRTMAX] = __SIGRTMIN,
>  };
>  static uint8_t target_to_host_signal_table[_NSIG];
>
> @@ -480,13 +474,18 @@ static int core_dump_signal(int sig)
>      }
>  }
>
> -void signal_init(void)
> +static void signal_table_init(void)
>  {
> -    TaskState *ts = (TaskState *)thread_cpu->opaque;
> -    struct sigaction act;
> -    struct sigaction oact;
>      int i, j;
> -    int host_sig;
> +
> +    /*
> +     * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
> +     * host libpthread signals.  This assumes no one actually uses SIGRTMAX :-
> /
> +     * To fix this properly we need to do manual signal delivery multiplexed
> +     * over a single host signal.
> +     */
> +    host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX;
> +    host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
>
>      /* generate signal conversion tables */
>      for(i = 1; i < _NSIG; i++) {
> @@ -497,14 +496,22 @@ void signal_init(void)
>          j = host_to_target_signal_table[i];

Since you are cleaning up this code, let's give this a more descriptive name - target_sig would be consistent with host_sig used elsewhere.

>          target_to_host_signal_table[j] = i;
>      }
> +}
> +
> +void signal_init(void)
> +{
> +    TaskState *ts = (TaskState *)thread_cpu->opaque;
> +    struct sigaction act;
> +    struct sigaction oact;
> +    int i;
> +    int host_sig;
> +
> +    /* initialize signal conversion tables */
> +    signal_table_init();
>
>      /* Set the signal mask from the host mask. */
>      sigprocmask(0, 0, &ts->signal_mask);
>
> -    /* set all host signal handlers. ALL signals are blocked during
> -       the handlers to serialize them. */
> -    memset(sigact_table, 0, sizeof(sigact_table));
> -
>      sigfillset(&act.sa_mask);
>      act.sa_flags = SA_SIGINFO;
>      act.sa_sigaction = host_signal_handler;
> --
> 2.24.1
>

RE: [PATCH 3/4] linux-user: fix TARGET_NSIG and _NSIG uses

[Taylor Simpson]

> -----Original Message-----
> From: Laurent Vivier <laurent@vivier.eu>
> Sent: Saturday, February 1, 2020 6:28 AM
> To: qemu-devel@nongnu.org
> Cc: Josh Kunz <jkz@google.com>; milos.stojanovic@rt-rk.com; Matus Kysel
> <mkysel@tachyum.com>; Aleksandar Markovic <aleksandar.markovic@rt-
> rk.com>; Marlies Ruck <marlies.ruck@gmail.com>; Laurent Vivier
> <laurent@vivier.eu>; Peter Maydell <peter.maydell@linaro.org>; Taylor
> Simpson <tsimpson@quicinc.com>; Riku Voipio <riku.voipio@iki.fi>
> Subject: [PATCH 3/4] linux-user: fix TARGET_NSIG and _NSIG uses
>
> Valid signal numbers are between 1 (SIGHUP) and SIGRTMAX.
>
> System includes define _NSIG to SIGRTMAX + 1, but QEMU (like kernel)
> defines TARGET_NSIG to TARGET_SIGRTMAX.
>
> Fix all the checks involving the signal range.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/signal.c | 51 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c index
> f42a2e1a82a5..3491f0a7ecb1 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -30,6 +30,15 @@ static struct target_sigaction
> sigact_table[TARGET_NSIG];  static void host_signal_handler(int
> host_signum, siginfo_t *info,
>                                  void *puc);
>
> +
> +/*
> + * System includes define _NSIG as SIGRTMAX + 1,
> + * but qemu (like the kernel) defines TARGET_NSIG as TARGET_SIGRTMAX
> + * and the first signal is SIGHUP defined as 1
> + * Signal number 0 is reserved for use as kill(pid, 0), to test whether
> + * a process exists without sending it a signal.
> + */
> +QEMU_BUILD_BUG_ON(__SIGRTMAX + 1 != _NSIG);
>  static uint8_t host_to_target_signal_table[_NSIG] = {
>      [SIGHUP] = TARGET_SIGHUP,
>      [SIGINT] = TARGET_SIGINT,
> @@ -67,19 +76,24 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
>      [SIGSYS] = TARGET_SIGSYS,
>      /* next signals stay the same */
>  };
> -static uint8_t target_to_host_signal_table[_NSIG];
>
> +static uint8_t target_to_host_signal_table[TARGET_NSIG + 1];
> +
> +/* valid sig is between 1 and _NSIG - 1 */
>  int host_to_target_signal(int sig)
>  {
> -    if (sig < 0 || sig >= _NSIG)
> +    if (sig < 1 || sig >= _NSIG) {
>          return sig;
> +    }
>      return host_to_target_signal_table[sig];  }
>
> +/* valid sig is between 1 and TARGET_NSIG */
>  int target_to_host_signal(int sig)
>  {
> -    if (sig < 0 || sig >= _NSIG)
> +    if (sig < 1 || sig > TARGET_NSIG) {
>          return sig;
> +    }
>      return target_to_host_signal_table[sig];  }
>
> @@ -100,11 +114,15 @@ static inline int target_sigismember(const
> target_sigset_t *set, int signum)  void
> host_to_target_sigset_internal(target_sigset_t *d,
>                                      const sigset_t *s)  {
> -    int i;
> +    int i, j;
>      target_sigemptyset(d);
> -    for (i = 1; i <= TARGET_NSIG; i++) {
> +    for (i = 1; i < _NSIG; i++) {
> +        j = host_to_target_signal(i);

More descriptive name - target_sig

> +        if (j < 1 || j > TARGET_NSIG) {
> +            continue;
> +        }
>          if (sigismember(s, i)) {
> -            target_sigaddset(d, host_to_target_signal(i));
> +            target_sigaddset(d, j);
>          }
>      }
>  }
> @@ -122,11 +140,15 @@ void host_to_target_sigset(target_sigset_t *d,
> const sigset_t *s)  void target_to_host_sigset_internal(sigset_t *d,
>                                      const target_sigset_t *s)  {
> -    int i;
> +    int i, j;
>      sigemptyset(d);
>      for (i = 1; i <= TARGET_NSIG; i++) {
> +        j = target_to_host_signal(i);

More descriptive name - host_sig

> +        if (j < 1 || j >= _NSIG) {
> +            continue;
> +        }
>          if (target_sigismember(s, i)) {
> -            sigaddset(d, target_to_host_signal(i));
> +            sigaddset(d, j);
>          }
>      }
>  }
> @@ -488,13 +510,14 @@ static void signal_table_init(void)
>      host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
>
>      /* generate signal conversion tables */
> -    for(i = 1; i < _NSIG; i++) {
> -        if (host_to_target_signal_table[i] == 0)
> +    for (i = 1; i < _NSIG; i++) {
> +        if (host_to_target_signal_table[i] == 0) {
>              host_to_target_signal_table[i] = i;
> -    }
> -    for(i = 1; i < _NSIG; i++) {
> +        }
>          j = host_to_target_signal_table[i];

More descriptive name - target_sig

> -        target_to_host_signal_table[j] = i;
> +        if (j <= TARGET_NSIG) {
> +            target_to_host_signal_table[j] = i;
> +        }
>      }
>  }
>
> @@ -517,7 +540,7 @@ void signal_init(void)
>      act.sa_sigaction = host_signal_handler;
>      for(i = 1; i <= TARGET_NSIG; i++) {  #ifdef TARGET_GPROF
> -        if (i == SIGPROF) {
> +        if (i == TARGET_SIGPROF) {
>              continue;
>          }
>  #endif
> --
> 2.24.1
>

RE: [PATCH 4/4] linux-user: fix use of SIGRTMIN

[Taylor Simpson]

> -----Original Message-----
> From: Laurent Vivier <laurent@vivier.eu>
> Sent: Saturday, February 1, 2020 6:28 AM
> To: qemu-devel@nongnu.org
> Cc: Josh Kunz <jkz@google.com>; milos.stojanovic@rt-rk.com; Matus Kysel
> <mkysel@tachyum.com>; Aleksandar Markovic <aleksandar.markovic@rt-
> rk.com>; Marlies Ruck <marlies.ruck@gmail.com>; Laurent Vivier
> <laurent@vivier.eu>; Peter Maydell <peter.maydell@linaro.org>; Taylor
> Simpson <tsimpson@quicinc.com>; Riku Voipio <riku.voipio@iki.fi>
> Subject: [PATCH 4/4] linux-user: fix use of SIGRTMIN
>
> Some RT signals can be in use by glibc,
> it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32).
>
> So SIGRTMIN cannot be mapped to TARGET_SIGRTMIN.
>
> Instead of swapping only SIGRTMIN and SIGRTMAX, map all the range
> [TARGET_SIGRTMIN ... TARGET_SIGRTMAX - X] to
>       [__SIGRTMIN + X ... SIGRTMAX ]
> (SIGRTMIN is __SIGRTMIN + X).
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/signal.c     | 34 ++++++++++++++++++++++++++++------
>  linux-user/trace-events |  3 +++
>  2 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c index
> 3491f0a7ecb1..c4abacde49a0 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -501,15 +501,20 @@ static void signal_table_init(void)
>      int i, j;
>
>      /*
> -     * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
> -     * host libpthread signals.  This assumes no one actually uses SIGRTMAX :-/
> -     * To fix this properly we need to do manual signal delivery multiplexed
> -     * over a single host signal.
> +     * some RT signals can be in use by glibc,
> +     * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32)
>       */
> -    host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX;
> -    host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
> +    for (i = SIGRTMIN; i <= SIGRTMAX; i++) {
> +        j = i - SIGRTMIN + TARGET_SIGRTMIN;
> +        if (j <= TARGET_NSIG) {
> +            host_to_target_signal_table[i] = j;
> +        }
> +    }
>
>      /* generate signal conversion tables */
> +    for (i = 1; i <= TARGET_NSIG; i++) {
> +        target_to_host_signal_table[i] = _NSIG; /* poison */
> +    }
>      for (i = 1; i < _NSIG; i++) {
>          if (host_to_target_signal_table[i] == 0) {
>              host_to_target_signal_table[i] = i; @@ -519,6 +524,15 @@ static void
> signal_table_init(void)
>              target_to_host_signal_table[j] = i;
>          }
>      }
> +
> +    if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) {
> +        for (i = 1, j = 0; i <= TARGET_NSIG; i++) {
> +            if (target_to_host_signal_table[i] == _NSIG) {
> +                j++;
> +            }
> +        }
> +        trace_signal_table_init(j);

It looks like j will have a count of the number of poison entries, but the message in trace_signal_table_init is "missing signal number".  Is that what you intend?

> +    }
>  }
>
>  void signal_init(void)
> @@ -817,6 +831,8 @@ int do_sigaction(int sig, const struct target_sigaction
> *act,
>      int host_sig;
>      int ret = 0;
>
> +    trace_signal_do_sigaction_guest(sig, TARGET_NSIG);

Shouldn't this be _NSIG, not TARGET_NSIG?

> +
>      if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig ==
> TARGET_SIGSTOP) {
>          return -TARGET_EINVAL;
>      }
> @@ -847,6 +863,12 @@ int do_sigaction(int sig, const struct target_sigaction
> *act,
>
>          /* we update the host linux signal state */
>          host_sig = target_to_host_signal(sig);
> +        trace_signal_do_sigaction_host(host_sig, TARGET_NSIG);
> +        if (host_sig > SIGRTMAX) {
> +            /* we don't have enough host signals to map all target signals */
> +            qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d\n",
> sig);
> +            return -TARGET_EINVAL;
> +        }
>          if (host_sig != SIGSEGV && host_sig != SIGBUS) {
>              sigfillset(&act1.sa_mask);
>              act1.sa_flags = SA_SIGINFO; diff --git a/linux-user/trace-events
> b/linux-user/trace-events index f6de1b8befc0..eb4b7701c400 100644
> --- a/linux-user/trace-events
> +++ b/linux-user/trace-events
> @@ -1,6 +1,9 @@
>  # See docs/devel/tracing.txt for syntax documentation.
>
>  # signal.c
> +signal_table_init(int i) "missing signal number: %d"
> +signal_do_sigaction_guest(int sig, int max) "target signal %d (MAX %d)"
> +signal_do_sigaction_host(int sig, int max) "host signal %d (MAX %d)"
>  # */signal.c
>  user_setup_frame(void *env, uint64_t frame_addr) "env=%p
> frame_addr=0x%"PRIx64  user_setup_rt_frame(void *env, uint64_t
> frame_addr) "env=%p frame_addr=0x%"PRIx64
> --
> 2.24.1
>

Re: [PATCH 0/4] linux-user: fix use of SIGRTMIN

[Josh Kunz]

On Sat, Feb 1, 2020 at 4:27 AM Laurent Vivier <laurent@vivier.eu> wrote:
> This has been tested with Go (golang 1.10.1 linux/arm64, bionic) on x86_64
> fedora 31. We can avoid the failure in this case allowing the unsupported
> signals when we don't provide the "act" parameters to sigaction, only the
> "oldact" one. I have also run the LTP suite with several target and debian
> based distros.

This breaks with go1.13+ (I also verified at hash 753d56d364)[1]. I
tested using an aarch64 guest on an x86 system, but this should
manifest on any architecture when the guest/host have the same number
of signals (and glibc reserves some host signals). From the traceback,
you can see it dies in `initsig` which is called at startup. Any Go
program should fail.

Since go does not use a libc, it assumes that all signals from
[1.._NSIG) are available[2], and will panic if it cannot do an
rt_sigaction for all of them. Go already has some special handling for
QEMU where it silently discards failing rt_sigaction calls to signals
32, 33, and 64 [3]. Since this patch reserves an extra signal for
__SIGRTMIN+1 as well, it blocks out guest signal 63 and Go fails to
initialize.

While I personally support this patch series (current handling of
guest glibc signals is broken), it *will* break Go binaries. I don't
know of a way to avoid this while supporting guest __SIGRTMIN+1,
without either doing true signal multiplexing, or patching Go.

[1] https://gist.github.com/joshkunz/b6c80724072cc1dce79a6253d40b016f
[2] https://github.com/golang/go/blob/67f0f83216930e053441500e2b28c3fa2b667581/src/runtime/signal_unix.go#L123
[3] https://github.com/golang/go/blob/master/src/runtime/os_linux.go#L466-L473

>
> Laurent Vivier (4):
>   linux-user: add missing TARGET_SIGRTMIN for hppa
>   linux-user: cleanup signal.c
>   linux-user: fix TARGET_NSIG and _NSIG uses
>   linux-user: fix use of SIGRTMIN
>
>  linux-user/hppa/target_signal.h |   1 +
>  linux-user/signal.c             | 110 +++++++++++++++++++++++---------
>  linux-user/trace-events         |   3 +
>  3 files changed, 85 insertions(+), 29 deletions(-)
>
> --
> 2.24.1
>

Re: [PATCH 0/4] linux-user: fix use of SIGRTMIN

[Laurent Vivier]

Le 04/02/2020 à 01:03, Josh Kunz a écrit :
> On Sat, Feb 1, 2020 at 4:27 AM Laurent Vivier <laurent@vivier.eu> wrote:
>> This has been tested with Go (golang 1.10.1 linux/arm64, bionic) on x86_64
>> fedora 31. We can avoid the failure in this case allowing the unsupported
>> signals when we don't provide the "act" parameters to sigaction, only the
>> "oldact" one. I have also run the LTP suite with several target and debian
>> based distros.
> 
> This breaks with go1.13+ (I also verified at hash 753d56d364)[1]. I
> tested using an aarch64 guest on an x86 system, but this should
> manifest on any architecture when the guest/host have the same number
> of signals (and glibc reserves some host signals). From the traceback,
> you can see it dies in `initsig` which is called at startup. Any Go
> program should fail.

I reproduced the problem with aarch64/eoan, go1.12.10. Thank you.

> Since go does not use a libc, it assumes that all signals from
> [1.._NSIG) are available[2], and will panic if it cannot do an
> rt_sigaction for all of them. Go already has some special handling for
> QEMU where it silently discards failing rt_sigaction calls to signals
> 32, 33, and 64 [3]. Since this patch reserves an extra signal for
> __SIGRTMIN+1 as well, it blocks out guest signal 63 and Go fails to
> initialize.

We should add signal 63 here, but it's becoming not very clean.

> While I personally support this patch series (current handling of
> guest glibc signals is broken), it *will* break Go binaries. I don't
> know of a way to avoid this while supporting guest __SIGRTMIN+1,
> without either doing true signal multiplexing, or patching Go.

I think we could also simply ignore the error. As returning an error is
generally an abort condition even if the signal is never used, and it's
what we would do in go to avoid the problem. We will have problem later
with programs that really want to use the signal but I don't think we
can do better (do we know programs using 31 RT signals? or starting by
the end of the list?).

something like:

diff --git a/linux-user/signal.c b/linux-user/signal.c
index c4abacde49a0..169a84afe90b 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -866,8 +866,8 @@ int do_sigaction(int sig, const struct
target_sigaction *act,
         trace_signal_do_sigaction_host(host_sig, TARGET_NSIG);
         if (host_sig > SIGRTMAX) {
             /* we don't have enough host signals to map all target
signals */
-            qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d\n",
sig);
-            return -TARGET_EINVAL;
+            qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d,
ignored", sig);
+            return 0;
         }


Thanks,
Laurent

Re: [PATCH 2/4] linux-user: cleanup signal.c

[Laurent Vivier]

Le 03/02/2020 à 23:58, Taylor Simpson a écrit :
> 
> 
>> -----Original Message-----
>> From: Laurent Vivier <laurent@vivier.eu>
>> Sent: Saturday, February 1, 2020 6:28 AM
>> To: qemu-devel@nongnu.org
>> Cc: Josh Kunz <jkz@google.com>; milos.stojanovic@rt-rk.com; Matus Kysel
>> <mkysel@tachyum.com>; Aleksandar Markovic <aleksandar.markovic@rt-
>> rk.com>; Marlies Ruck <marlies.ruck@gmail.com>; Laurent Vivier
>> <laurent@vivier.eu>; Peter Maydell <peter.maydell@linaro.org>; Taylor
>> Simpson <tsimpson@quicinc.com>; Riku Voipio <riku.voipio@iki.fi>
>> Subject: [PATCH 2/4] linux-user: cleanup signal.c
>>
>> -------------------------------------------------------------------------
>> CAUTION: This email originated from outside of the organization.
>> -------------------------------------------------------------------------
>>
>> No functionnal changes. Prepare the field for future fixes.
> 
> 
> Spelling error

Sorry, french word. Will be changed by "functional"

> 
>>
>> Remove memset(.., 0, ...) that is useless on a static array
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>  linux-user/signal.c | 37 ++++++++++++++++++++++---------------
>>  1 file changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c index
>> 5ca6d62b15d3..f42a2e1a82a5 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -66,12 +66,6 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
>>      [SIGPWR] = TARGET_SIGPWR,
>>      [SIGSYS] = TARGET_SIGSYS,
>>      /* next signals stay the same */
>> -    /* Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
>> -       host libpthread signals.  This assumes no one actually uses SIGRTMAX :-/
>> -       To fix this properly we need to do manual signal delivery multiplexed
>> -       over a single host signal.  */
>> -    [__SIGRTMIN] = __SIGRTMAX,
>> -    [__SIGRTMAX] = __SIGRTMIN,
>>  };
>>  static uint8_t target_to_host_signal_table[_NSIG];
>>
>> @@ -480,13 +474,18 @@ static int core_dump_signal(int sig)
>>      }
>>  }
>>
>> -void signal_init(void)
>> +static void signal_table_init(void)
>>  {
>> -    TaskState *ts = (TaskState *)thread_cpu->opaque;
>> -    struct sigaction act;
>> -    struct sigaction oact;
>>      int i, j;
>> -    int host_sig;
>> +
>> +    /*
>> +     * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
>> +     * host libpthread signals.  This assumes no one actually uses SIGRTMAX :-
>> /
>> +     * To fix this properly we need to do manual signal delivery multiplexed
>> +     * over a single host signal.
>> +     */
>> +    host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX;
>> +    host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
>>
>>      /* generate signal conversion tables */
>>      for(i = 1; i < _NSIG; i++) {
>> @@ -497,14 +496,22 @@ void signal_init(void)
>>          j = host_to_target_signal_table[i];
> 
> Since you are cleaning up this code, let's give this a more descriptive name - target_sig would be consistent with host_sig used elsewhere.

I agree.

Thanks,
Laurent

Re: [PATCH 4/4] linux-user: fix use of SIGRTMIN

[Laurent Vivier]

Le 04/02/2020 à 00:15, Taylor Simpson a écrit :
> 
> 
>> -----Original Message-----
>> From: Laurent Vivier <laurent@vivier.eu>
>> Sent: Saturday, February 1, 2020 6:28 AM
>> To: qemu-devel@nongnu.org
>> Cc: Josh Kunz <jkz@google.com>; milos.stojanovic@rt-rk.com; Matus Kysel
>> <mkysel@tachyum.com>; Aleksandar Markovic <aleksandar.markovic@rt-
>> rk.com>; Marlies Ruck <marlies.ruck@gmail.com>; Laurent Vivier
>> <laurent@vivier.eu>; Peter Maydell <peter.maydell@linaro.org>; Taylor
>> Simpson <tsimpson@quicinc.com>; Riku Voipio <riku.voipio@iki.fi>
>> Subject: [PATCH 4/4] linux-user: fix use of SIGRTMIN
>>
>> Some RT signals can be in use by glibc,
>> it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32).
>>
>> So SIGRTMIN cannot be mapped to TARGET_SIGRTMIN.
>>
>> Instead of swapping only SIGRTMIN and SIGRTMAX, map all the range
>> [TARGET_SIGRTMIN ... TARGET_SIGRTMAX - X] to
>>       [__SIGRTMIN + X ... SIGRTMAX ]
>> (SIGRTMIN is __SIGRTMIN + X).
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>  linux-user/signal.c     | 34 ++++++++++++++++++++++++++++------
>>  linux-user/trace-events |  3 +++
>>  2 files changed, 31 insertions(+), 6 deletions(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c index
>> 3491f0a7ecb1..c4abacde49a0 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -501,15 +501,20 @@ static void signal_table_init(void)
>>      int i, j;
>>
>>      /*
>> -     * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
>> -     * host libpthread signals.  This assumes no one actually uses SIGRTMAX :-/
>> -     * To fix this properly we need to do manual signal delivery multiplexed
>> -     * over a single host signal.
>> +     * some RT signals can be in use by glibc,
>> +     * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32)
>>       */
>> -    host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX;
>> -    host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
>> +    for (i = SIGRTMIN; i <= SIGRTMAX; i++) {
>> +        j = i - SIGRTMIN + TARGET_SIGRTMIN;
>> +        if (j <= TARGET_NSIG) {
>> +            host_to_target_signal_table[i] = j;
>> +        }
>> +    }
>>
>>      /* generate signal conversion tables */
>> +    for (i = 1; i <= TARGET_NSIG; i++) {
>> +        target_to_host_signal_table[i] = _NSIG; /* poison */
>> +    }
>>      for (i = 1; i < _NSIG; i++) {
>>          if (host_to_target_signal_table[i] == 0) {
>>              host_to_target_signal_table[i] = i; @@ -519,6 +524,15 @@ static void
>> signal_table_init(void)
>>              target_to_host_signal_table[j] = i;
>>          }
>>      }
>> +
>> +    if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) {
>> +        for (i = 1, j = 0; i <= TARGET_NSIG; i++) {
>> +            if (target_to_host_signal_table[i] == _NSIG) {
>> +                j++;
>> +            }
>> +        }
>> +        trace_signal_table_init(j);
> 
> It looks like j will have a count of the number of poison entries, but the message in trace_signal_table_init is "missing signal number".  Is that what you intend?

Yes, poison entries are the entries that cannot be used for the guest.
Perhaps it would be more correct as "Number of missing signals:"?

> 
>> +    }
>>  }
>>
>>  void signal_init(void)
>> @@ -817,6 +831,8 @@ int do_sigaction(int sig, const struct target_sigaction
>> *act,
>>      int host_sig;
>>      int ret = 0;
>>
>> +    trace_signal_do_sigaction_guest(sig, TARGET_NSIG);
> 
> Shouldn't this be _NSIG, not TARGET_NSIG?

No: do_sigaction() takes a number from the guest, so "sig" is a target
signal number, and this trace displays also the maximum value for the
target.

Thanks,
Laurent

Re: [PATCH 0/4] linux-user: fix use of SIGRTMIN

[Josh Kunz]

On Tue, Feb 4, 2020 at 3:55 AM Laurent Vivier <laurent@vivier.eu> wrote:
> We should add signal 63 here, but it's becoming not very clean.

https://golang.org/issue/33746 has some discussion of the issue. The
Go maintainers wanted to clean this up rather than just adding 63. The
patch is on ice right now because it was unclear if QEMU was going to
add a breaking patch. Now that QEMU has this behavior, I think they
would be willing to accept it, or something similar.

> I think we could also simply ignore the error. As returning an error is
> generally an abort condition even if the signal is never used, and it's
> what we would do in go to avoid the problem. We will have problem later
> with programs that really want to use the signal but I don't think we
> can do better (do we know programs using 31 RT signals? or starting by
> the end of the list?).

My general sense is that RT signals are very rarely used, and the only
uses I can find are based off SIGRTMIN. This sounds reasonable to me.

Josh