[PATCH v3 0/3] linux-user: Detect and report host crashes

[PATCH v3 0/3] linux-user: Detect and report host crashes

[Richard Henderson]

Supercedes: 20230812164314.352131-1-deller@gmx.de
("[PATCH] Fix signal handler to detect crashes in qemu-linux-user")

Fixes the issues I pointed out vs Helge's v2.


r~


Helge Deller (1):
  linux-user: Detect and report host crashes

Richard Henderson (2):
  linux-user: Split out die_with_signal
  linux-user: Exit not abort in die_with_backtrace

 linux-user/signal.c | 106 +++++++++++++++++++++++++++++++-------------
 1 file changed, 76 insertions(+), 30 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] linux-user: Split out die_with_signal

[Richard Henderson]

Because we trap so many signals for use by the guest,
we have to take extra steps to exit properly.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/signal.c | 52 ++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 748a98f3e5..e1cd111a1b 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -694,6 +694,33 @@ void cpu_loop_exit_sigbus(CPUState *cpu, target_ulong addr,
 }
 
 /* abort execution with signal */
+static G_NORETURN
+void die_with_signal(int sig)
+{
+    struct sigaction act = {
+        .sa_handler = SIG_DFL,
+    };
+
+    /*
+     * The proper exit code for dying from an uncaught signal is -<signal>.
+     * The kernel doesn't allow exit() or _exit() to pass a negative value.
+     * To get the proper exit code we need to actually die from an uncaught
+     * signal.  Here the default signal handler is installed, we send
+     * the signal and we wait for it to arrive.
+     */
+    sigfillset(&act.sa_mask);
+    sigaction(sig, &act, NULL);
+
+    kill(getpid(), sig);
+
+    /* Make sure the signal isn't masked (reusing the mask inside of act). */
+    sigdelset(&act.sa_mask, sig);
+    sigsuspend(&act.sa_mask);
+
+    /* unreachable */
+    abort();
+}
+
 static G_NORETURN
 void dump_core_and_abort(CPUArchState *cpu_env, int target_sig)
 {
@@ -701,7 +728,6 @@ void dump_core_and_abort(CPUArchState *cpu_env, int target_sig)
     CPUArchState *env = cpu->env_ptr;
     TaskState *ts = (TaskState *)cpu->opaque;
     int host_sig, core_dumped = 0;
-    struct sigaction act;
 
     host_sig = target_to_host_signal(target_sig);
     trace_user_dump_core_and_abort(env, target_sig, host_sig);
@@ -725,29 +751,7 @@ void dump_core_and_abort(CPUArchState *cpu_env, int target_sig)
     }
 
     preexit_cleanup(cpu_env, 128 + target_sig);
-
-    /* The proper exit code for dying from an uncaught signal is
-     * -<signal>.  The kernel doesn't allow exit() or _exit() to pass
-     * a negative value.  To get the proper exit code we need to
-     * actually die from an uncaught signal.  Here the default signal
-     * handler is installed, we send ourself a signal and we wait for
-     * it to arrive. */
-    sigfillset(&act.sa_mask);
-    act.sa_handler = SIG_DFL;
-    act.sa_flags = 0;
-    sigaction(host_sig, &act, NULL);
-
-    /* For some reason raise(host_sig) doesn't send the signal when
-     * statically linked on x86-64. */
-    kill(getpid(), host_sig);
-
-    /* Make sure the signal isn't masked (just reuse the mask inside
-    of act) */
-    sigdelset(&act.sa_mask, host_sig);
-    sigsuspend(&act.sa_mask);
-
-    /* unreachable */
-    abort();
+    die_with_signal(host_sig);
 }
 
 /* queue a signal so that it will be send to the virtual CPU as soon
-- 
2.34.1

Re: [PATCH v3 1/3] linux-user: Split out die_with_signal

[Philippe Mathieu-Daudé]

On 22/8/23 22:34, Richard Henderson wrote:
> Because we trap so many signals for use by the guest,
> we have to take extra steps to exit properly.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/signal.c | 52 ++++++++++++++++++++++++---------------------
>   1 file changed, 28 insertions(+), 24 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 2/3] linux-user: Exit not abort in die_with_backtrace

[Richard Henderson]

This line is supposed to be unreachable, but if we're going to
have it at all, SIGABRT via abort() is subject to the same signal
peril that created this function in the first place.

We can _exit immediately without peril.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index e1cd111a1b..57fbf2f58b 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -718,7 +718,7 @@ void die_with_signal(int sig)
     sigsuspend(&act.sa_mask);
 
     /* unreachable */
-    abort();
+    _exit(EXIT_FAILURE);
 }
 
 static G_NORETURN
-- 
2.34.1

Re: [PATCH v3 2/3] linux-user: Exit not abort in die_with_backtrace

[Philippe Mathieu-Daudé]

On 22/8/23 22:34, Richard Henderson wrote:
> This line is supposed to be unreachable, but if we're going to
> have it at all, SIGABRT via abort() is subject to the same signal
> peril that created this function in the first place.

:)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> We can _exit immediately without peril.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/signal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

[PATCH v3 3/3] linux-user: Detect and report host crashes

[Richard Henderson]

From: Helge Deller <deller@gmx.de>

If there is an internal program error in the qemu source code which
raises SIGSEGV or SIGBUS, we currently assume the signal belongs to
the guest.

With an artificial error introduced, we will now print

QEMU internal SIGSEGV {si_code=1, si_addr=(nil)}
QEMU v8.1.50 target x86_64 running /usr/bin/cat
QEMU backtrace:
    ./qemu-x86_64(+0xf83d9) [0x55c48587a3d9]
    ./qemu-x86_64(+0xf8663) [0x55c48587a663]
    /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7f39eee42520]
    ./qemu-x86_64(+0x1132fa) [0x55c4858952fa]
    ./qemu-x86_64(+0x11396f) [0x55c48589596f]
    ./qemu-x86_64(+0x1140ef) [0x55c4858960ef]
    ./qemu-x86_64(+0x115796) [0x55c485897796]
    ./qemu-x86_64(+0x11f9dd) [0x55c4858a19dd]
    ./qemu-x86_64(+0x3f8da) [0x55c4857c18da]
    ./qemu-x86_64(+0xf3bfb) [0x55c485875bfb]
    /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f39eee29d90]
    /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7f39eee29e40]
    ./qemu-x86_64(+0x39075) [0x55c4857bb075]
Segmentation fault

Signed-off-by: Helge Deller <deller@gmx.de>
Message-Id: <20230812164314.352131-1-deller@gmx.de>
[rth: Use in_code_gen_buffer and die_with_signal]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/signal.c | 54 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 57fbf2f58b..1ffd1354c3 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -23,6 +23,7 @@
 
 #include <sys/ucontext.h>
 #include <sys/resource.h>
+#include <execinfo.h>
 
 #include "qemu.h"
 #include "user-internals.h"
@@ -32,6 +33,7 @@
 #include "signal-common.h"
 #include "host-signal.h"
 #include "user/safe-syscall.h"
+#include "tcg/tcg.h"
 
 static struct target_sigaction sigact_table[TARGET_NSIG];
 
@@ -785,6 +787,34 @@ static inline void rewind_if_in_safe_syscall(void *puc)
     }
 }
 
+static G_NORETURN
+void die_with_backtrace(siginfo_t *info)
+{
+    void *array[20];
+    int size;
+
+    fprintf(stderr,
+            "QEMU internal SIG%s {si_code=%d, si_addr=%p}\n"
+            "QEMU v" QEMU_VERSION " target " UNAME_MACHINE " running %s\n",
+            sigabbrev_np(info->si_signo), info->si_code, info->si_addr,
+            exec_path);
+
+    size = backtrace(array, ARRAY_SIZE(array));
+    if (size) {
+        char **strings = backtrace_symbols(array, size);
+        if (strings) {
+            fprintf(stderr, "QEMU backtrace:\n");
+            for (int i = 0; i < size; ++i) {
+                fprintf(stderr, "    %s\n", strings[i]);
+            }
+            free(strings);
+        }
+    }
+
+    preexit_cleanup(thread_cpu->env_ptr, TARGET_SIGKILL);
+    die_with_signal(info->si_signo);
+}
+
 static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
 {
     CPUArchState *env = thread_cpu->env_ptr;
@@ -820,16 +850,28 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
         is_write = host_signal_write(info, uc);
         access_type = adjust_signal_pc(&pc, is_write);
 
+        /* If this was a write to a TB protected page, restart. */
+        if (is_write
+            && host_sig == SIGSEGV
+            && info->si_code == SEGV_ACCERR
+            && h2g_valid(host_addr)
+            && handle_sigsegv_accerr_write(cpu, sigmask, pc, guest_addr)) {
+            return;
+        }
+
+        /*
+         * If the access was not on behalf of the guest, within the executable
+         * mapping of the generated code buffer, then it is a host bug.
+         */
+        if (access_type != MMU_INST_FETCH
+            && !in_code_gen_buffer((void *)(pc - tcg_splitwx_diff))) {
+            die_with_backtrace(info);
+        }
+
         if (host_sig == SIGSEGV) {
             bool maperr = true;
 
             if (info->si_code == SEGV_ACCERR && h2g_valid(host_addr)) {
-                /* If this was a write to a TB protected page, restart. */
-                if (is_write &&
-                    handle_sigsegv_accerr_write(cpu, sigmask, pc, guest_addr)) {
-                    return;
-                }
-
                 /*
                  * With reserved_va, the whole address space is PROT_NONE,
                  * which means that we may get ACCERR when we want MAPERR.
-- 
2.34.1