[PATCH 0/3] user: recursive signal delivery fix and test

[PATCH 0/3] user: recursive signal delivery fix and test

[Nicholas Piggin]

I found this recursive delivery problem when testing new riscv vector
state support patches (plus a minor lock user balance bug).

This or similar type of issue was fixed with 8bd3773cce1 but I couldn't
see exactly what scenario that was addressing. Is there any other way a
signal can be generated during handle_pending_signal()? The SIGTTIN
case, perhaps?

Thanks,
Nick

Nicholas Piggin (3):
  linux-user: sigaltstack lock_user_struct missing unlock
  bsd-user, linux-user: signal: recursive signal delivery fix
  tests/tcg: add recursive signal delivery tests

 bsd-user/signal.c            |  10 +-
 linux-user/signal.c          |  36 +++---
 tests/tcg/multiarch/badsig.c | 226 +++++++++++++++++++++++++++++++++++
 3 files changed, 252 insertions(+), 20 deletions(-)
 create mode 100644 tests/tcg/multiarch/badsig.c

-- 
2.51.0

[PATCH 1/3] linux-user: sigaltstack lock_user_struct missing unlock

[Nicholas Piggin]

lock_user_struct is missing a corresponding unlock.

(bsd-user does correctly unlock in the equivalent code.)

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 linux-user/signal.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 804096bd44..e4b8b28bfe 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -292,11 +292,10 @@ void target_save_altstack(target_stack_t *uss, CPUArchState *env)
     __put_user(ts->sigaltstack_used.ss_size, &uss->ss_size);
 }
 
-abi_long target_restore_altstack(target_stack_t *uss, CPUArchState *env)
+abi_long target_restore_altstack(target_stack_t *ss, CPUArchState *env)
 {
     TaskState *ts = get_task_state(thread_cpu);
     size_t minstacksize = TARGET_MINSIGSTKSZ;
-    target_stack_t ss;
 
 #if defined(TARGET_PPC64)
     /* ELF V2 for PPC64 has a 4K minimum stack size for signal handlers */
@@ -306,33 +305,29 @@ abi_long target_restore_altstack(target_stack_t *uss, CPUArchState *env)
     }
 #endif
 
-    __get_user(ss.ss_sp, &uss->ss_sp);
-    __get_user(ss.ss_size, &uss->ss_size);
-    __get_user(ss.ss_flags, &uss->ss_flags);
-
     if (on_sig_stack(get_sp_from_cpustate(env))) {
         return -TARGET_EPERM;
     }
 
-    switch (ss.ss_flags) {
+    switch (ss->ss_flags) {
     default:
         return -TARGET_EINVAL;
 
     case TARGET_SS_DISABLE:
-        ss.ss_size = 0;
-        ss.ss_sp = 0;
+        ss->ss_sp = 0;
+        ss->ss_size = 0;
         break;
 
     case TARGET_SS_ONSTACK:
     case 0:
-        if (ss.ss_size < minstacksize) {
+        if (ss->ss_size < minstacksize) {
             return -TARGET_ENOMEM;
         }
         break;
     }
 
-    ts->sigaltstack_used.ss_sp = ss.ss_sp;
-    ts->sigaltstack_used.ss_size = ss.ss_size;
+    ts->sigaltstack_used.ss_sp = ss->ss_sp;
+    ts->sigaltstack_used.ss_size = ss->ss_size;
     return 0;
 }
 
@@ -1140,11 +1135,17 @@ abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr,
 
     if (uss_addr) {
         target_stack_t *uss;
+        target_stack_t ss;
 
         if (!lock_user_struct(VERIFY_READ, uss, uss_addr, 1)) {
             goto out;
         }
-        ret = target_restore_altstack(uss, env);
+        __get_user(ss.ss_sp, &uss->ss_sp);
+        __get_user(ss.ss_size, &uss->ss_size);
+        __get_user(ss.ss_flags, &uss->ss_flags);
+        unlock_user_struct(uss, uss_addr, 0);
+
+        ret = target_restore_altstack(&ss, env);
         if (ret) {
             goto out;
         }
-- 
2.51.0

[PATCH 2/3] bsd-user, linux-user: signal: recursive signal delivery fix

[Nicholas Piggin]

Synchronous signals must accommodate a synchronous signal being
raised during delivery, as asynchronous ones do. For example
badframe errors during delivery will cause SIGSEGV to be raised.

Without this fix, cpu_loop() runs process_pending_signals() which
delivers the first synchronous signal (e.g., SIGILL) which fails
to set the handler and forces SIGSEGV, but that is not picked up.
process_pending_signals() returns. Then cpu_loop() runs cpu_exec()
again, which attempts to execute the same instruction, another
SIGILL.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 bsd-user/signal.c   | 10 ++++++----
 linux-user/signal.c |  9 ++++++---
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/bsd-user/signal.c b/bsd-user/signal.c
index dadcc037dc..3e5e41e1b1 100644
--- a/bsd-user/signal.c
+++ b/bsd-user/signal.c
@@ -998,7 +998,12 @@ void process_pending_signals(CPUArchState *env)
                 sigdelset(&ts->signal_mask, target_to_host_signal(sig));
                 sigact_table[sig - 1]._sa_handler = TARGET_SIG_DFL;
             }
+            /*
+             * Restart scan from the beginning, as handle_pending_signal
+             * might have resulted in a new synchronous signal (eg SIGSEGV).
+             */
             handle_pending_signal(env, sig, &ts->sync_signal);
+            goto restart_scan;
         }
 
         k = ts->sigtab;
@@ -1008,10 +1013,7 @@ void process_pending_signals(CPUArchState *env)
             if (k->pending &&
                 !sigismember(blocked_set, target_to_host_signal(sig))) {
                 handle_pending_signal(env, sig, k);
-                /*
-                 * Restart scan from the beginning, as handle_pending_signal
-                 * might have resulted in a new synchronous signal (eg SIGSEGV).
-                 */
+                /* Restart scan, explained above. */
                 goto restart_scan;
             }
         }
diff --git a/linux-user/signal.c b/linux-user/signal.c
index e4b8b28bfe..9d43e080ce 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1385,6 +1385,11 @@ void process_pending_signals(CPUArchState *cpu_env)
             }
 
             handle_pending_signal(cpu_env, sig, &ts->sync_signal);
+            /*
+             * Restart scan from the beginning, as handle_pending_signal
+             * might have resulted in a new synchronous signal (eg SIGSEGV).
+             */
+            goto restart_scan;
         }
 
         for (sig = 1; sig <= TARGET_NSIG; sig++) {
@@ -1395,9 +1400,7 @@ void process_pending_signals(CPUArchState *cpu_env)
                 (!sigismember(blocked_set,
                               target_to_host_signal_table[sig]))) {
                 handle_pending_signal(cpu_env, sig, &ts->sigtab[sig - 1]);
-                /* Restart scan from the beginning, as handle_pending_signal
-                 * might have resulted in a new synchronous signal (eg SIGSEGV).
-                 */
+                /* Restart scan, explained above. */
                 goto restart_scan;
             }
         }
-- 
2.51.0

[PATCH 3/3] tests/tcg: add recursive signal delivery tests

[Nicholas Piggin]

Add tests which exercise a synchronous signal being raised in
synchronous and asynchronous signal delivery paths. It does this by
delivering them with an altstack that is made inaccessible, then
catching the badframe SIGSEGV with a handler on the regular stack.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/tcg/multiarch/badsig.c | 226 +++++++++++++++++++++++++++++++++++
 1 file changed, 226 insertions(+)
 create mode 100644 tests/tcg/multiarch/badsig.c

diff --git a/tests/tcg/multiarch/badsig.c b/tests/tcg/multiarch/badsig.c
new file mode 100644
index 0000000000..0f346f4147
--- /dev/null
+++ b/tests/tcg/multiarch/badsig.c
@@ -0,0 +1,226 @@
+/*
+ * linux-user "badframe" signal handling tests.
+ *
+ * Copyright (c) 2026 Tenstorrent USA, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Test "badframe" signal handling paths, which force a
+ * SIGSEGV signal from the signal handler setup code,
+ * which tests the recursive signal "restart_scan" logic
+ * in process_pending_signals().
+ */
+
+#include <stdarg.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <assert.h>
+#include <errno.h>
+#include <pthread.h>
+#include <string.h>
+#include <signal.h>
+#include <setjmp.h>
+#include <time.h>
+#include <sys/time.h>
+#include <sys/mman.h>
+
+#undef DEBUG
+#ifdef DEBUG
+#define dprintf(...) printf(__VA_ARGS__)
+#else
+#define dprintf(...)
+#endif
+
+static void error1(const char *filename, int line, const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    fprintf(stderr, "%s:%d: ", filename, line);
+    vfprintf(stderr, fmt, ap);
+    fprintf(stderr, "\n");
+    va_end(ap);
+    exit(1);
+}
+
+static int __chk_error(const char *filename, int line, int ret)
+{
+    if (ret < 0) {
+        error1(filename, line, "%m (ret=%d, errno=%d/%s)",
+               ret, errno, strerror(errno));
+    }
+    return ret;
+}
+
+#define error(fmt, ...) error1(__FILE__, __LINE__, fmt, ## __VA_ARGS__)
+
+#define chk_error(ret) __chk_error(__FILE__, __LINE__, (ret))
+
+static bool do_siglongjmp;
+static sigjmp_buf current_sigjmp_buf;
+
+static volatile int total_alarm_count;
+
+static void sig_alarm(int sig, siginfo_t *info, void *puc)
+{
+    if (sig != SIGRTMIN) {
+        error("unexpected signal");
+    }
+    dprintf("SIGRTMIN\n");
+    total_alarm_count++;
+}
+
+static volatile int total_segv_count;
+
+static void sig_segv(int sig, siginfo_t *info, void *puc)
+{
+    if (sig != SIGSEGV) {
+        error("unexpected signal");
+    }
+    dprintf("SIGSEGV\n");
+    total_segv_count++;
+    if (do_siglongjmp) {
+        dprintf("siglongjmp()\n");
+        siglongjmp(current_sigjmp_buf, 1);
+    }
+}
+
+static volatile int total_trap_count;
+
+static void sig_trap(int sig, siginfo_t *info, void *puc)
+{
+    if (sig == SIGTRAP) {
+        dprintf("SIGTRAP\n");
+    } else if (sig == SIGILL) {
+        dprintf("SIGILL\n");
+    } else if (sig == SIGABRT) {
+        dprintf("SIGABRT\n");
+    } else {
+        error("unexpected signal");
+    }
+    total_trap_count++;
+    if (do_siglongjmp) {
+        dprintf("siglongjmp()\n");
+        siglongjmp(current_sigjmp_buf, 1);
+    }
+}
+
+static void test_signals(void)
+{
+    struct sigaction act;
+    struct itimerspec it;
+    timer_t tid;
+    struct sigevent sev;
+    stack_t ss;
+    void *mem;
+
+    /* Set up SEGV handler */
+    act.sa_sigaction = sig_segv;
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = SA_SIGINFO;
+    chk_error(sigaction(SIGSEGV, &act, NULL));
+
+    /* Set up an altstack */
+    mem = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (mem == MAP_FAILED) {
+            fprintf(stderr, "out of memory");
+            exit(EXIT_FAILURE);
+    }
+
+    ss.ss_sp = mem;
+    ss.ss_flags = 0;
+    ss.ss_size = SIGSTKSZ;
+    chk_error(sigaltstack(&ss, NULL));
+
+    /* Async signal test */
+
+    /* Set up RTMIN handler on alt stack */
+    act.sa_sigaction = sig_alarm;
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = SA_SIGINFO | SA_ONSTACK;
+    chk_error(sigaction(SIGRTMIN, &act, NULL));
+
+    /* Create POSIX timer */
+    sev.sigev_notify = SIGEV_SIGNAL;
+    sev.sigev_signo = SIGRTMIN;
+    sev.sigev_value.sival_ptr = &tid;
+    chk_error(timer_create(CLOCK_REALTIME, &sev, &tid));
+
+    it.it_interval.tv_sec = 0;
+    it.it_interval.tv_nsec = 1000000;
+    it.it_value.tv_sec = 0;
+    it.it_value.tv_nsec = 1000000;
+    chk_error(timer_settime(tid, 0, &it, NULL));
+
+    while (total_alarm_count == 0) {
+        usleep(1000);
+    }
+    total_alarm_count = 0;
+
+    chk_error(timer_delete(tid));
+
+    assert(total_segv_count == 0);
+
+    /* Make the alt stack bad */
+    chk_error(mprotect(mem, SIGSTKSZ, PROT_NONE));
+
+    chk_error(timer_create(CLOCK_REALTIME, &sev, &tid));
+    chk_error(timer_settime(tid, 0, &it, NULL));
+
+    while (total_segv_count == 0) {
+        usleep(1000);
+    }
+    total_segv_count = 0;
+
+    chk_error(timer_delete(tid));
+
+    assert(total_alarm_count == 0);
+
+    /* Make the alt stack good */
+    chk_error(mprotect(mem, SIGSTKSZ, PROT_READ | PROT_WRITE));
+
+    /* Bad sync signal test */
+
+    /* Set up SIGILL/TRAP/ABRT handler on alt stack */
+    act.sa_sigaction = sig_trap;
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = SA_SIGINFO | SA_ONSTACK;
+    chk_error(sigaction(SIGTRAP, &act, NULL));
+    chk_error(sigaction(SIGILL, &act, NULL));
+    chk_error(sigaction(SIGABRT, &act, NULL));
+
+    if (sigsetjmp(current_sigjmp_buf, 1) == 0) {
+        do_siglongjmp = true;
+        /* Cause a synchronous signal */
+        dprintf("__builtin_trap()\n");
+        __builtin_trap();
+        assert(0);
+    }
+    do_siglongjmp = false;
+    assert(total_trap_count == 1);
+    total_trap_count = 0;
+    assert(total_segv_count == 0);
+
+    /* Make the alt stack bad */
+    chk_error(mprotect(mem, SIGSTKSZ, PROT_NONE));
+
+    if (sigsetjmp(current_sigjmp_buf, 1) == 0) {
+        do_siglongjmp = true;
+        /* Cause a synchronous signal */
+        dprintf("__builtin_trap()\n");
+        __builtin_trap();
+        assert(0);
+    }
+    do_siglongjmp = false;
+    assert(total_segv_count == 1);
+    total_segv_count = 0;
+    assert(total_trap_count == 0);
+}
+
+int main(int argc, char **argv)
+{
+    test_signals();
+    return 0;
+}
-- 
2.51.0

Re: [PATCH 2/3] bsd-user, linux-user: signal: recursive signal delivery fix

[Warner Losh]

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

On Sat, Mar 21, 2026 at 7:56 AM Nicholas Piggin <npiggin@gmail.com> wrote:

> Synchronous signals must accommodate a synchronous signal being
> raised during delivery, as asynchronous ones do. For example
> badframe errors during delivery will cause SIGSEGV to be raised.
>
> Without this fix, cpu_loop() runs process_pending_signals() which
> delivers the first synchronous signal (e.g., SIGILL) which fails
> to set the handler and forces SIGSEGV, but that is not picked up.
> process_pending_signals() returns. Then cpu_loop() runs cpu_exec()
> again, which attempts to execute the same instruction, another
> SIGILL.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  bsd-user/signal.c   | 10 ++++++----
>  linux-user/signal.c |  9 ++++++---
>  2 files changed, 12 insertions(+), 7 deletions(-)
>

Reviewed-by: Warner Losh <imp@bsdimp.com>

Interesting edge case...

Warner


> diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> index dadcc037dc..3e5e41e1b1 100644
> --- a/bsd-user/signal.c
> +++ b/bsd-user/signal.c
> @@ -998,7 +998,12 @@ void process_pending_signals(CPUArchState *env)
>                  sigdelset(&ts->signal_mask, target_to_host_signal(sig));
>                  sigact_table[sig - 1]._sa_handler = TARGET_SIG_DFL;
>              }
> +            /*
> +             * Restart scan from the beginning, as handle_pending_signal
> +             * might have resulted in a new synchronous signal (eg
> SIGSEGV).
> +             */
>              handle_pending_signal(env, sig, &ts->sync_signal);
> +            goto restart_scan;
>          }
>
>          k = ts->sigtab;
> @@ -1008,10 +1013,7 @@ void process_pending_signals(CPUArchState *env)
>              if (k->pending &&
>                  !sigismember(blocked_set, target_to_host_signal(sig))) {
>                  handle_pending_signal(env, sig, k);
> -                /*
> -                 * Restart scan from the beginning, as
> handle_pending_signal
> -                 * might have resulted in a new synchronous signal (eg
> SIGSEGV).
> -                 */
> +                /* Restart scan, explained above. */
>                  goto restart_scan;
>              }
>          }
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index e4b8b28bfe..9d43e080ce 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1385,6 +1385,11 @@ void process_pending_signals(CPUArchState *cpu_env)
>              }
>
>              handle_pending_signal(cpu_env, sig, &ts->sync_signal);
> +            /*
> +             * Restart scan from the beginning, as handle_pending_signal
> +             * might have resulted in a new synchronous signal (eg
> SIGSEGV).
> +             */
> +            goto restart_scan;
>          }
>
>          for (sig = 1; sig <= TARGET_NSIG; sig++) {
> @@ -1395,9 +1400,7 @@ void process_pending_signals(CPUArchState *cpu_env)
>                  (!sigismember(blocked_set,
>                                target_to_host_signal_table[sig]))) {
>                  handle_pending_signal(cpu_env, sig, &ts->sigtab[sig - 1]);
> -                /* Restart scan from the beginning, as
> handle_pending_signal
> -                 * might have resulted in a new synchronous signal (eg
> SIGSEGV).
> -                 */
> +                /* Restart scan, explained above. */
>                  goto restart_scan;
>              }
>          }
> --
> 2.51.0
>
>

[-- Attachment #2: Type: text/html, Size: 4626 bytes --]