* [PATCH v4 00/10] linux-user: Detect and report host crashes
@ 2023-08-23 5:16 Richard Henderson
2023-08-23 5:16 ` [PATCH v4 01/10] linux-user: Split out die_with_signal Richard Henderson
` (10 more replies)
0 siblings, 11 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
More signal cleanups. Mostly tested by temporarily adding an
abort, divide by zero, undefined instruction, null dereference,
within the implementation of a guest syscall to induce an error.
r~
Helge Deller (1):
linux-user: Detect and report host crashes
Richard Henderson (9):
linux-user: Split out die_with_signal
linux-user: Exit not abort in die_with_backtrace
linux-user: Use die_with_signal with abort
linux-user: Only register handlers for core_dump_signal by default
linux-user: Map unsupported signals to an out-of-bounds value
linux-user: Remap SIGPROF when CONFIG_GPROF
linux-user: Simplify signal_init
linux-user: Split out host_sig{segv,bus}_handler
linux-user: Detect and report host SIGILL, SIGFPE, SIGTRAP
linux-user/signal.c | 400 ++++++++++++++++++++++++++------------------
1 file changed, 240 insertions(+), 160 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 01/10] linux-user: Split out die_with_signal
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 12:55 ` Philippe Mathieu-Daudé
2023-08-23 5:16 ` [PATCH v4 02/10] linux-user: Exit not abort in die_with_backtrace Richard Henderson
` (9 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 02/10] linux-user: Exit not abort in die_with_backtrace
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
2023-08-23 5:16 ` [PATCH v4 01/10] linux-user: Split out die_with_signal Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 12:55 ` Philippe Mathieu-Daudé
2023-08-23 5:16 ` [PATCH v4 03/10] linux-user: Use die_with_signal with abort Richard Henderson
` (8 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 03/10] linux-user: Use die_with_signal with abort
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
2023-08-23 5:16 ` [PATCH v4 01/10] linux-user: Split out die_with_signal Richard Henderson
2023-08-23 5:16 ` [PATCH v4 02/10] linux-user: Exit not abort in die_with_backtrace Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 5:16 ` [PATCH v4 04/10] linux-user: Detect and report host crashes Richard Henderson
` (7 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
Describe the convoluted way by which we can see
ERROR:../alt/accel/tcg/cpu-exec.c:532:cpu_exec_longjmp_cleanup: assertion failed: (cpu == current_cpu)
Bail out! ERROR:../alt/accel/tcg/cpu-exec.c:532:cpu_exec_longjmp_cleanup: assertion failed: (cpu == current_cpu)
for encountering an an abort() in qemu source.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
linux-user/signal.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 57fbf2f58b..a77d003de6 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -721,6 +721,18 @@ void die_with_signal(int sig)
_exit(EXIT_FAILURE);
}
+/*
+ * The system abort() will raise SIGABRT, which will get caught and deferred
+ * by host_signal_handler. Returning into system abort will try harder.
+ * Eventually, on x86, it will execute HLT, which raises SIGSEGV. This goes
+ * back into host_signal_handler, through a different path which may longjmp
+ * back to the main loop. This often explodes.
+ */
+void abort(void)
+{
+ die_with_signal(SIGABRT);
+}
+
static G_NORETURN
void dump_core_and_abort(CPUArchState *cpu_env, int target_sig)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 04/10] linux-user: Detect and report host crashes
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
` (2 preceding siblings ...)
2023-08-23 5:16 ` [PATCH v4 03/10] linux-user: Use die_with_signal with abort Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 5:16 ` [PATCH v4 05/10] linux-user: Only register handlers for core_dump_signal by default Richard Henderson
` (6 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
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]
Aborted
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 | 53 ++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 47 insertions(+), 6 deletions(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index a77d003de6..529fea8bba 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];
@@ -797,6 +799,33 @@ 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);
+ }
+ }
+
+ abort();
+}
+
static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
{
CPUArchState *env = thread_cpu->env_ptr;
@@ -832,16 +861,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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 05/10] linux-user: Only register handlers for core_dump_signal by default
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
` (3 preceding siblings ...)
2023-08-23 5:16 ` [PATCH v4 04/10] linux-user: Detect and report host crashes Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 5:16 ` [PATCH v4 06/10] linux-user: Map unsupported signals to an out-of-bounds value Richard Henderson
` (5 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
The set of fatal signals is really immaterial. If one arrives,
and is unhandled, then the qemu process dies and the parent gets
the correct signal.
It is only for those signals which we would like to perform a
guest core dump instead of a host core dump that we need to catch.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
linux-user/signal.c | 43 ++++++++++++++-----------------------------
1 file changed, 14 insertions(+), 29 deletions(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 529fea8bba..73f40699ad 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -489,26 +489,6 @@ void target_to_host_siginfo(siginfo_t *info, const target_siginfo_t *tinfo)
info->si_value.sival_ptr = (void *)(long)sival_ptr;
}
-static int fatal_signal (int sig)
-{
- switch (sig) {
- case TARGET_SIGCHLD:
- case TARGET_SIGURG:
- case TARGET_SIGWINCH:
- /* Ignored by default. */
- return 0;
- case TARGET_SIGCONT:
- case TARGET_SIGSTOP:
- case TARGET_SIGTSTP:
- case TARGET_SIGTTIN:
- case TARGET_SIGTTOU:
- /* Job control signals. */
- return 0;
- default:
- return 1;
- }
-}
-
/* returns 1 if given signal should dump core if not handled */
static int core_dump_signal(int sig)
{
@@ -608,8 +588,9 @@ void signal_init(void)
SIGSEGV and SIGBUS, to detect exceptions. We can not just
trap all signals because it affects syscall interrupt
behavior. But do trap all default-fatal signals. */
- if (fatal_signal (i))
+ if (core_dump_signal(i)) {
sigaction(host_sig, &act, NULL);
+ }
}
}
@@ -999,7 +980,6 @@ int do_sigaction(int sig, const struct target_sigaction *act,
struct target_sigaction *oact, abi_ulong ka_restorer)
{
struct target_sigaction *k;
- struct sigaction act1;
int host_sig;
int ret = 0;
@@ -1059,22 +1039,27 @@ int do_sigaction(int sig, const struct target_sigaction *act,
return 0;
}
if (host_sig != SIGSEGV && host_sig != SIGBUS) {
+ struct sigaction act1;
+
sigfillset(&act1.sa_mask);
act1.sa_flags = SA_SIGINFO;
- if (k->sa_flags & TARGET_SA_RESTART)
- act1.sa_flags |= SA_RESTART;
- /* NOTE: it is important to update the host kernel signal
- ignore state to avoid getting unexpected interrupted
- syscalls */
if (k->_sa_handler == TARGET_SIG_IGN) {
+ /*
+ * It is important to update the host kernel signal ignore
+ * state to avoid getting unexpected interrupted syscalls.
+ */
act1.sa_sigaction = (void *)SIG_IGN;
} else if (k->_sa_handler == TARGET_SIG_DFL) {
- if (fatal_signal (sig))
+ if (core_dump_signal(sig)) {
act1.sa_sigaction = host_signal_handler;
- else
+ } else {
act1.sa_sigaction = (void *)SIG_DFL;
+ }
} else {
act1.sa_sigaction = host_signal_handler;
+ if (k->sa_flags & TARGET_SA_RESTART) {
+ act1.sa_flags |= SA_RESTART;
+ }
}
ret = sigaction(host_sig, &act1, NULL);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 06/10] linux-user: Map unsupported signals to an out-of-bounds value
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
` (4 preceding siblings ...)
2023-08-23 5:16 ` [PATCH v4 05/10] linux-user: Only register handlers for core_dump_signal by default Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 5:16 ` [PATCH v4 07/10] linux-user: Remap SIGPROF when CONFIG_GPROF Richard Henderson
` (4 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
Do not return a valid signal number in one domain
when given an invalid signal number in the other domain.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
linux-user/signal.c | 72 ++++++++++++++++++++++++---------------------
1 file changed, 38 insertions(+), 34 deletions(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 73f40699ad..9d16e3c8c5 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -45,9 +45,8 @@ abi_ulong default_sigreturn;
abi_ulong default_rt_sigreturn;
/*
- * 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
+ * System includes define _NSIG as SIGRTMAX + 1, but qemu (like the kernel)
+ * defines TARGET_NSIG as TARGET_SIGRTMAX and the first signal is 1.
* Signal number 0 is reserved for use as kill(pid, 0), to test whether
* a process exists without sending it a signal.
*/
@@ -58,7 +57,6 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
#define MAKE_SIG_ENTRY(sig) [sig] = TARGET_##sig,
MAKE_SIGNAL_LIST
#undef MAKE_SIG_ENTRY
- /* next signals stay the same */
};
static uint8_t target_to_host_signal_table[TARGET_NSIG + 1];
@@ -66,18 +64,24 @@ 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 < 1 || sig >= _NSIG) {
+ if (sig < 1) {
return sig;
}
+ if (sig >= _NSIG) {
+ return TARGET_NSIG + 1;
+ }
return host_to_target_signal_table[sig];
}
/* valid sig is between 1 and TARGET_NSIG */
int target_to_host_signal(int sig)
{
- if (sig < 1 || sig > TARGET_NSIG) {
+ if (sig < 1) {
return sig;
}
+ if (sig > TARGET_NSIG) {
+ return _NSIG;
+ }
return target_to_host_signal_table[sig];
}
@@ -508,48 +512,48 @@ static int core_dump_signal(int sig)
static void signal_table_init(void)
{
- int host_sig, target_sig, count;
+ int hsig, tsig, count;
/*
* Signals are supported starting from TARGET_SIGRTMIN and going up
- * until we run out of host realtime signals.
- * glibc at least uses only the lower 2 rt signals and probably
- * nobody's using the upper ones.
- * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32)
- * To fix this properly we need to do manual signal delivery multiplexed
- * over a single host signal.
+ * until we run out of host realtime signals. Glibc uses the lower 2
+ * RT signals and (hopefully) nobody uses the upper ones.
+ * This is why SIGRTMIN (34) is generally greater than __SIGRTMIN (32).
+ * To fix this properly we would need to do manual signal delivery
+ * multiplexed over a single host signal.
* Attempts for configure "missing" signals via sigaction will be
* silently ignored.
*/
- for (host_sig = SIGRTMIN; host_sig <= SIGRTMAX; host_sig++) {
- target_sig = host_sig - SIGRTMIN + TARGET_SIGRTMIN;
- if (target_sig <= TARGET_NSIG) {
- host_to_target_signal_table[host_sig] = target_sig;
+ for (hsig = SIGRTMIN; hsig <= SIGRTMAX; hsig++) {
+ tsig = hsig - SIGRTMIN + TARGET_SIGRTMIN;
+ if (tsig <= TARGET_NSIG) {
+ host_to_target_signal_table[hsig] = tsig;
}
}
- /* generate signal conversion tables */
- for (target_sig = 1; target_sig <= TARGET_NSIG; target_sig++) {
- target_to_host_signal_table[target_sig] = _NSIG; /* poison */
- }
- for (host_sig = 1; host_sig < _NSIG; host_sig++) {
- if (host_to_target_signal_table[host_sig] == 0) {
- host_to_target_signal_table[host_sig] = host_sig;
- }
- target_sig = host_to_target_signal_table[host_sig];
- if (target_sig <= TARGET_NSIG) {
- target_to_host_signal_table[target_sig] = host_sig;
+ /* Invert the mapping that has already been assigned. */
+ for (hsig = 1; hsig < _NSIG; hsig++) {
+ tsig = host_to_target_signal_table[hsig];
+ if (tsig) {
+ assert(target_to_host_signal_table[tsig] == 0);
+ target_to_host_signal_table[tsig] = hsig;
}
}
- if (trace_event_get_state_backends(TRACE_SIGNAL_TABLE_INIT)) {
- for (target_sig = 1, count = 0; target_sig <= TARGET_NSIG; target_sig++) {
- if (target_to_host_signal_table[target_sig] == _NSIG) {
- count++;
- }
+ /* Map everything else out-of-bounds. */
+ for (hsig = 1; hsig < _NSIG; hsig++) {
+ if (host_to_target_signal_table[hsig] == 0) {
+ host_to_target_signal_table[hsig] = TARGET_NSIG + 1;
}
- trace_signal_table_init(count);
}
+ for (count = 0, tsig = 1; tsig <= TARGET_NSIG; tsig++) {
+ if (target_to_host_signal_table[tsig] == 0) {
+ target_to_host_signal_table[tsig] = _NSIG;
+ count++;
+ }
+ }
+
+ trace_signal_table_init(count);
}
void signal_init(void)
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 07/10] linux-user: Remap SIGPROF when CONFIG_GPROF
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
` (5 preceding siblings ...)
2023-08-23 5:16 ` [PATCH v4 06/10] linux-user: Map unsupported signals to an out-of-bounds value Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 5:16 ` [PATCH v4 08/10] linux-user: Simplify signal_init Richard Henderson
` (3 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
Protect the host's use of SIGPROF by remapping TARGET_SIGPROF
to one of the RT signals.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
linux-user/signal.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 9d16e3c8c5..b8913968cc 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -523,8 +523,15 @@ static void signal_table_init(void)
* multiplexed over a single host signal.
* Attempts for configure "missing" signals via sigaction will be
* silently ignored.
+ *
+ * If the host is using gprof, treat SIGPROF the same way.
*/
- for (hsig = SIGRTMIN; hsig <= SIGRTMAX; hsig++) {
+ hsig = SIGRTMIN;
+#ifdef CONFIG_GPROF
+ host_to_target_signal_table[SIGPROF] = 0;
+ host_to_target_signal_table[hsig++] = TARGET_SIGPROF;
+#endif
+ for (; hsig <= SIGRTMAX; hsig++) {
tsig = hsig - SIGRTMIN + TARGET_SIGRTMIN;
if (tsig <= TARGET_NSIG) {
host_to_target_signal_table[hsig] = tsig;
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 08/10] linux-user: Simplify signal_init
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
` (6 preceding siblings ...)
2023-08-23 5:16 ` [PATCH v4 07/10] linux-user: Remap SIGPROF when CONFIG_GPROF Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 5:16 ` [PATCH v4 09/10] linux-user: Split out host_sig{segv,bus}_handler Richard Henderson
` (2 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
Install the host signal handler at the same time we are
probing the target signals for SIG_IGN/SIG_DFL. Ignore
unmapped target signals.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
linux-user/signal.c | 47 +++++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 25 deletions(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index b8913968cc..a6917eadd8 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -566,10 +566,7 @@ static void signal_table_init(void)
void signal_init(void)
{
TaskState *ts = (TaskState *)thread_cpu->opaque;
- struct sigaction act;
- struct sigaction oact;
- int i;
- int host_sig;
+ struct sigaction act, oact;
/* initialize signal conversion tables */
signal_table_init();
@@ -580,28 +577,28 @@ void signal_init(void)
sigfillset(&act.sa_mask);
act.sa_flags = SA_SIGINFO;
act.sa_sigaction = host_signal_handler;
- for(i = 1; i <= TARGET_NSIG; i++) {
-#ifdef CONFIG_GPROF
- if (i == TARGET_SIGPROF) {
- continue;
- }
-#endif
- host_sig = target_to_host_signal(i);
- sigaction(host_sig, NULL, &oact);
- if (oact.sa_sigaction == (void *)SIG_IGN) {
- sigact_table[i - 1]._sa_handler = TARGET_SIG_IGN;
- } else if (oact.sa_sigaction == (void *)SIG_DFL) {
- sigact_table[i - 1]._sa_handler = TARGET_SIG_DFL;
- }
- /* If there's already a handler installed then something has
- gone horribly wrong, so don't even try to handle that case. */
- /* Install some handlers for our own use. We need at least
- SIGSEGV and SIGBUS, to detect exceptions. We can not just
- trap all signals because it affects syscall interrupt
- behavior. But do trap all default-fatal signals. */
- if (core_dump_signal(i)) {
- sigaction(host_sig, &act, NULL);
+
+ /*
+ * A parent process may configure ignored signals, but all other
+ * signals are default. For any target signals that have no host
+ * mapping, set to ignore. For all core_dump_signal, install our
+ * host signal handler so that we may invoke dump_core_and_abort.
+ * This includes SIGSEGV and SIGBUS, which are also need our signal
+ * handler for paging and exceptions.
+ */
+ for (int tsig = 1; tsig <= TARGET_NSIG; tsig++) {
+ int hsig = target_to_host_signal(tsig);
+ abi_ptr thand = TARGET_SIG_IGN;
+
+ if (hsig < _NSIG) {
+ struct sigaction *iact = core_dump_signal(tsig) ? &act : NULL;
+
+ sigaction(hsig, iact, &oact);
+ if (oact.sa_sigaction != (void *)SIG_IGN) {
+ thand = TARGET_SIG_DFL;
+ }
}
+ sigact_table[tsig - 1]._sa_handler = thand;
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 09/10] linux-user: Split out host_sig{segv,bus}_handler
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
` (7 preceding siblings ...)
2023-08-23 5:16 ` [PATCH v4 08/10] linux-user: Simplify signal_init Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-08-23 5:16 ` [PATCH v4 10/10] linux-user: Detect and report host SIGILL, SIGFPE, SIGTRAP Richard Henderson
2023-09-09 19:12 ` [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
10 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
Make host_signal_handler slightly easier to read.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
linux-user/signal.c | 145 ++++++++++++++++++++++++++------------------
1 file changed, 85 insertions(+), 60 deletions(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index a6917eadd8..68ceb2e4bd 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -815,6 +815,80 @@ void die_with_backtrace(siginfo_t *info)
abort();
}
+static void host_sigsegv_handler(CPUState *cpu, siginfo_t *info,
+ host_sigcontext *uc)
+{
+ uintptr_t host_addr = (uintptr_t)info->si_addr;
+ /*
+ * Convert forcefully to guest address space: addresses outside
+ * reserved_va are still valid to report via SEGV_MAPERR.
+ */
+ bool is_valid = h2g_valid(host_addr);
+ abi_ptr guest_addr = h2g_nocheck(host_addr);
+ uintptr_t pc = host_signal_pc(uc);
+ bool is_write = host_signal_write(info, uc);
+ MMUAccessType access_type = adjust_signal_pc(&pc, is_write);
+ bool maperr;
+
+ /* If this was a write to a TB protected page, restart. */
+ if (is_write
+ && is_valid
+ && info->si_code == SEGV_ACCERR
+ && handle_sigsegv_accerr_write(cpu, host_signal_mask(uc),
+ 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);
+ }
+
+ maperr = true;
+ if (is_valid && info->si_code == SEGV_ACCERR) {
+ /*
+ * With reserved_va, the whole address space is PROT_NONE,
+ * which means that we may get ACCERR when we want MAPERR.
+ */
+ if (page_get_flags(guest_addr) & PAGE_VALID) {
+ maperr = false;
+ } else {
+ info->si_code = SEGV_MAPERR;
+ }
+ }
+
+ sigprocmask(SIG_SETMASK, host_signal_mask(uc), NULL);
+ cpu_loop_exit_sigsegv(cpu, guest_addr, access_type, maperr, pc);
+}
+
+static void host_sigbus_handler(CPUState *cpu, siginfo_t *info,
+ host_sigcontext *uc)
+{
+ uintptr_t pc = host_signal_pc(uc);
+ bool is_write = host_signal_write(info, uc);
+ MMUAccessType access_type = adjust_signal_pc(&pc, is_write);
+
+ /*
+ * 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 (!in_code_gen_buffer((void *)(pc - tcg_splitwx_diff))) {
+ die_with_backtrace(info);
+ }
+
+ if (info->si_code == BUS_ADRALN) {
+ uintptr_t host_addr = (uintptr_t)info->si_addr;
+ abi_ptr guest_addr = h2g_nocheck(host_addr);
+
+ sigprocmask(SIG_SETMASK, host_signal_mask(uc), NULL);
+ cpu_loop_exit_sigbus(cpu, guest_addr, access_type, pc);
+ }
+}
+
static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
{
CPUArchState *env = thread_cpu->env_ptr;
@@ -826,73 +900,23 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
int guest_sig;
uintptr_t pc = 0;
bool sync_sig = false;
- void *sigmask = host_signal_mask(uc);
+ void *sigmask;
/*
* Non-spoofed SIGSEGV and SIGBUS are synchronous, and need special
* handling wrt signal blocking and unwinding.
*/
- if ((host_sig == SIGSEGV || host_sig == SIGBUS) && info->si_code > 0) {
- MMUAccessType access_type;
- uintptr_t host_addr;
- abi_ptr guest_addr;
- bool is_write;
-
- host_addr = (uintptr_t)info->si_addr;
-
- /*
- * Convert forcefully to guest address space: addresses outside
- * reserved_va are still valid to report via SEGV_MAPERR.
- */
- guest_addr = h2g_nocheck(host_addr);
-
- pc = host_signal_pc(uc);
- 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)) {
+ if (info->si_code > 0) {
+ switch (host_sig) {
+ case SIGSEGV:
+ /* Only returns on handle_sigsegv_accerr_write success. */
+ host_sigsegv_handler(cpu, info, uc);
return;
+ case SIGBUS:
+ host_sigbus_handler(cpu, info, uc);
+ sync_sig = true;
+ break;
}
-
- /*
- * 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)) {
- /*
- * With reserved_va, the whole address space is PROT_NONE,
- * which means that we may get ACCERR when we want MAPERR.
- */
- if (page_get_flags(guest_addr) & PAGE_VALID) {
- maperr = false;
- } else {
- info->si_code = SEGV_MAPERR;
- }
- }
-
- sigprocmask(SIG_SETMASK, sigmask, NULL);
- cpu_loop_exit_sigsegv(cpu, guest_addr, access_type, maperr, pc);
- } else {
- sigprocmask(SIG_SETMASK, sigmask, NULL);
- if (info->si_code == BUS_ADRALN) {
- cpu_loop_exit_sigbus(cpu, guest_addr, access_type, pc);
- }
- }
-
- sync_sig = true;
}
/* get target signal number */
@@ -933,6 +957,7 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
* would write 0xff bytes off the end of the structure and trash
* data on the struct.
*/
+ sigmask = host_signal_mask(uc);
memset(sigmask, 0xff, SIGSET_T_SIZE);
sigdelset(sigmask, SIGSEGV);
sigdelset(sigmask, SIGBUS);
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 10/10] linux-user: Detect and report host SIGILL, SIGFPE, SIGTRAP
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
` (8 preceding siblings ...)
2023-08-23 5:16 ` [PATCH v4 09/10] linux-user: Split out host_sig{segv,bus}_handler Richard Henderson
@ 2023-08-23 5:16 ` Richard Henderson
2023-09-09 19:12 ` [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
10 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-08-23 5:16 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
These signals, when not spoofed via kill(), are always bugs.
Use die_with_backtrace to report this sensibly.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
linux-user/signal.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 68ceb2e4bd..6d13b5c210 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -904,7 +904,8 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
/*
* Non-spoofed SIGSEGV and SIGBUS are synchronous, and need special
- * handling wrt signal blocking and unwinding.
+ * handling wrt signal blocking and unwinding. Non-spoofed SIGILL,
+ * SIGFPE, SIGTRAP are always host bugs.
*/
if (info->si_code > 0) {
switch (host_sig) {
@@ -916,6 +917,10 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
host_sigbus_handler(cpu, info, uc);
sync_sig = true;
break;
+ case SIGILL:
+ case SIGFPE:
+ case SIGTRAP:
+ die_with_backtrace(info);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v4 01/10] linux-user: Split out die_with_signal
2023-08-23 5:16 ` [PATCH v4 01/10] linux-user: Split out die_with_signal Richard Henderson
@ 2023-08-23 12:55 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-23 12:55 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: deller
On 23/8/23 07:16, 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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 02/10] linux-user: Exit not abort in die_with_backtrace
2023-08-23 5:16 ` [PATCH v4 02/10] linux-user: Exit not abort in die_with_backtrace Richard Henderson
@ 2023-08-23 12:55 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-23 12:55 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: deller
On 23/8/23 07:16, 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.
>
> 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(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
` (9 preceding siblings ...)
2023-08-23 5:16 ` [PATCH v4 10/10] linux-user: Detect and report host SIGILL, SIGFPE, SIGTRAP Richard Henderson
@ 2023-09-09 19:12 ` Richard Henderson
2023-09-12 9:45 ` Helge Deller
10 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2023-09-09 19:12 UTC (permalink / raw)
To: qemu-devel; +Cc: deller
Ping.
On 8/22/23 22:16, Richard Henderson wrote:
> More signal cleanups. Mostly tested by temporarily adding an
> abort, divide by zero, undefined instruction, null dereference,
> within the implementation of a guest syscall to induce an error.
>
>
> r~
>
>
> Helge Deller (1):
> linux-user: Detect and report host crashes
>
> Richard Henderson (9):
> linux-user: Split out die_with_signal
> linux-user: Exit not abort in die_with_backtrace
> linux-user: Use die_with_signal with abort
> linux-user: Only register handlers for core_dump_signal by default
> linux-user: Map unsupported signals to an out-of-bounds value
> linux-user: Remap SIGPROF when CONFIG_GPROF
> linux-user: Simplify signal_init
> linux-user: Split out host_sig{segv,bus}_handler
> linux-user: Detect and report host SIGILL, SIGFPE, SIGTRAP
>
> linux-user/signal.c | 400 ++++++++++++++++++++++++++------------------
> 1 file changed, 240 insertions(+), 160 deletions(-)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-09 19:12 ` [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
@ 2023-09-12 9:45 ` Helge Deller
2023-09-12 10:34 ` Michael Tokarev
0 siblings, 1 reply; 24+ messages in thread
From: Helge Deller @ 2023-09-12 9:45 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
On 9/9/23 21:12, Richard Henderson wrote:
> On 8/22/23 22:16, Richard Henderson wrote:
>> More signal cleanups. Mostly tested by temporarily adding an
>> abort, divide by zero, undefined instruction, null dereference,
>> within the implementation of a guest syscall to induce an error.
Applied on top of git head, linking fails when builing the *static* qemu-user binary:
/usr/bin/ld: /usr/lib/gcc/x86_64-redhat-linux/13/../../../../lib64/libc.a(abort.o): in function `abort':
(.text.unlikely+0x0): multiple definition of `abort'; libqemu-aarch64-linux-user.fa.p/linux-user_signal.c.o:/srv/_build/../../home/cvs/qemu/qemu/linux-user/signal.c:723: first defined here
Helge
>>
>> Helge Deller (1):
>> linux-user: Detect and report host crashes
>>
>> Richard Henderson (9):
>> linux-user: Split out die_with_signal
>> linux-user: Exit not abort in die_with_backtrace
>> linux-user: Use die_with_signal with abort
>> linux-user: Only register handlers for core_dump_signal by default
>> linux-user: Map unsupported signals to an out-of-bounds value
>> linux-user: Remap SIGPROF when CONFIG_GPROF
>> linux-user: Simplify signal_init
>> linux-user: Split out host_sig{segv,bus}_handler
>> linux-user: Detect and report host SIGILL, SIGFPE, SIGTRAP
>>
>> linux-user/signal.c | 400 ++++++++++++++++++++++++++------------------
>> 1 file changed, 240 insertions(+), 160 deletions(-)
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-12 9:45 ` Helge Deller
@ 2023-09-12 10:34 ` Michael Tokarev
2023-09-18 14:05 ` Helge Deller
0 siblings, 1 reply; 24+ messages in thread
From: Michael Tokarev @ 2023-09-12 10:34 UTC (permalink / raw)
To: Helge Deller, Richard Henderson, qemu-devel
12.09.2023 12:45, Helge Deller:
> /usr/bin/ld: /usr/lib/gcc/x86_64-redhat-linux/13/../../../../lib64/libc.a(abort.o): in function `abort':
> (.text.unlikely+0x0): multiple definition of `abort';
> libqemu-aarch64-linux-user.fa.p/linux-user_signal.c.o:/srv/_build/../../home/cvs/qemu/qemu/linux-user/signal.c:723: first defined here
[PATCH v4 03/10] linux-user: Use die_with_signal with abort
Sigh.
I'd be double-cautious when overriding system functions like this, - it's
almost always a bad idea.
/mjt
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-12 10:34 ` Michael Tokarev
@ 2023-09-18 14:05 ` Helge Deller
2023-09-19 7:40 ` Michael Tokarev
2023-09-19 8:29 ` Richard Henderson
0 siblings, 2 replies; 24+ messages in thread
From: Helge Deller @ 2023-09-18 14:05 UTC (permalink / raw)
To: Richard Henderson, qemu-devel, Michael Tokarev
On 9/12/23 12:34, Michael Tokarev wrote:
> 12.09.2023 12:45, Helge Deller:
>
>> /usr/bin/ld: /usr/lib/gcc/x86_64-redhat-linux/13/../../../../lib64/libc.a(abort.o): in function `abort':
>> (.text.unlikely+0x0): multiple definition of `abort'; libqemu-aarch64-linux-user.fa.p/linux-user_signal.c.o:/srv/_build/../../home/cvs/qemu/qemu/linux-user/signal.c:723: first defined here
>
> [PATCH v4 03/10] linux-user: Use die_with_signal with abort
>
> Sigh.
>
> I'd be double-cautious when overriding system functions like this, - it's
> almost always a bad idea.
Richard, I'm not sure, but with that change:
-void abort(void)
+void __attribute__((weak)) abort(void)
it will at least successfully link the binary. Not sure which effects it has,
but probably not worse than before your patch series...
Helge
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-18 14:05 ` Helge Deller
@ 2023-09-19 7:40 ` Michael Tokarev
2023-09-19 8:00 ` Helge Deller
2023-09-19 8:29 ` Richard Henderson
1 sibling, 1 reply; 24+ messages in thread
From: Michael Tokarev @ 2023-09-19 7:40 UTC (permalink / raw)
To: Helge Deller, Richard Henderson, qemu-devel
18.09.2023 17:05, Helge Deller wrote:
> On 9/12/23 12:34, Michael Tokarev wrote:
>> 12.09.2023 12:45, Helge Deller:
>>
>>> /usr/bin/ld: /usr/lib/gcc/x86_64-redhat-linux/13/../../../../lib64/libc.a(abort.o): in function `abort':
>>> (.text.unlikely+0x0): multiple definition of `abort';
>>> libqemu-aarch64-linux-user.fa.p/linux-user_signal.c.o:/srv/_build/../../home/cvs/qemu/qemu/linux-user/signal.c:723: first defined here
>>
>> [PATCH v4 03/10] linux-user: Use die_with_signal with abort
>>
>> Sigh.
>>
>> I'd be double-cautious when overriding system functions like this, - it's
>> almost always a bad idea.
>
> Richard, I'm not sure, but with that change:
>
> -void abort(void)
> +void __attribute__((weak)) abort(void)
>
> it will at least successfully link the binary. Not sure which effects it has,
> but probably not worse than before your patch series...
With weak here, ld should peak abort() from glibc, basically reverting this
patch:
linux-user: Use die_with_signal with abort
+/*
+ * The system abort() will raise SIGABRT, which will get caught and deferred
+ * by host_signal_handler. Returning into system abort will try harder.
+ * Eventually, on x86, it will execute HLT, which raises SIGSEGV. This goes
+ * back into host_signal_handler, through a different path which may longjmp
+ * back to the main loop. This often explodes.
+ */
+void abort(void)
+{
+ die_with_signal(SIGABRT);
+}
+
I think it's better to revert it now.
Probably the right solution is to use qemu_abort() (and qemu_assert() etc),
and maybe #define abort(x) qemu_abort(x). Even if some way to redefine
abort like the above will work on glibc, it does not mean it will work
on *bsd and in other contexts.
Yes, providing its own abort() is tempting due to its simplicity.
But it is a grey area at best.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-19 7:40 ` Michael Tokarev
@ 2023-09-19 8:00 ` Helge Deller
2023-09-19 8:26 ` Michael Tokarev
0 siblings, 1 reply; 24+ messages in thread
From: Helge Deller @ 2023-09-19 8:00 UTC (permalink / raw)
To: Michael Tokarev, Richard Henderson, qemu-devel
On 9/19/23 09:40, Michael Tokarev wrote:
> 18.09.2023 17:05, Helge Deller wrote:
>> On 9/12/23 12:34, Michael Tokarev wrote:
>>> 12.09.2023 12:45, Helge Deller:
>>>
>>>> /usr/bin/ld: /usr/lib/gcc/x86_64-redhat-linux/13/../../../../lib64/libc.a(abort.o): in function `abort':
>>>> (.text.unlikely+0x0): multiple definition of `abort'; libqemu-aarch64-linux-user.fa.p/linux-user_signal.c.o:/srv/_build/../../home/cvs/qemu/qemu/linux-user/signal.c:723: first defined here
>>>
>>> [PATCH v4 03/10] linux-user: Use die_with_signal with abort
>>>
>>> Sigh.
>>>
>>> I'd be double-cautious when overriding system functions like this, - it's
>>> almost always a bad idea.
>>
>> Richard, I'm not sure, but with that change:
>>
>> -void abort(void)
>> +void __attribute__((weak)) abort(void)
>>
>> it will at least successfully link the binary. Not sure which effects it has,
>> but probably not worse than before your patch series...
>
> With weak here, ld should peak abort() from glibc, basically reverting this
> patch:
>
> linux-user: Use die_with_signal with abort
> +/*
> + * The system abort() will raise SIGABRT, which will get caught and deferred
> + * by host_signal_handler. Returning into system abort will try harder.
> + * Eventually, on x86, it will execute HLT, which raises SIGSEGV. This goes
> + * back into host_signal_handler, through a different path which may longjmp
> + * back to the main loop. This often explodes.
> + */
> +void abort(void)
> +{
> + die_with_signal(SIGABRT);
> +}
> +
>
> I think it's better to revert it now.
Maybe.
I only did static compile builds, and not even for all platforms.
Since I assume Richard did test it in his builds (which probably were
linux-user built as dynamic executable) seem to have worked for him,
the code probably works on those binaries.
If this function is left out on static builds (because libc.a already
provides this function), then there is no difference (aka same not-optimal
behaviour) as without this patch.
So, keeping this patch may help for dynamic linux-user executables at least.
> Probably the right solution is to use qemu_abort() (and qemu_assert() etc),
> and maybe #define abort(x) qemu_abort(x). Even if some way to redefine
> abort like the above will work on glibc, it does not mean it will work
> on *bsd and in other contexts.
True. That's probably the better solution.
> Yes, providing its own abort() is tempting due to its simplicity.
> But it is a grey area at best.
Helge
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-19 8:00 ` Helge Deller
@ 2023-09-19 8:26 ` Michael Tokarev
2023-09-19 8:38 ` Richard Henderson
0 siblings, 1 reply; 24+ messages in thread
From: Michael Tokarev @ 2023-09-19 8:26 UTC (permalink / raw)
To: Helge Deller, Richard Henderson, qemu-devel
19.09.2023 11:00, Helge Deller wrote:
..
>> Probably the right solution is to use qemu_abort() (and qemu_assert() etc),
>> and maybe #define abort(x) qemu_abort(x). Even if some way to redefine
>> abort like the above will work on glibc, it does not mean it will work
>> on *bsd and in other contexts.
>
> True. That's probably the better solution.
That wont work, since abort() gets called from a lot of libraries
(gilbc has 1000s of calls to it)
Sigh.
/mjt
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-18 14:05 ` Helge Deller
2023-09-19 7:40 ` Michael Tokarev
@ 2023-09-19 8:29 ` Richard Henderson
1 sibling, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2023-09-19 8:29 UTC (permalink / raw)
To: Helge Deller, qemu-devel, Michael Tokarev
On 9/18/23 16:05, Helge Deller wrote:
> On 9/12/23 12:34, Michael Tokarev wrote:
>> 12.09.2023 12:45, Helge Deller:
>>
>>> /usr/bin/ld: /usr/lib/gcc/x86_64-redhat-linux/13/../../../../lib64/libc.a(abort.o): in
>>> function `abort':
>>> (.text.unlikely+0x0): multiple definition of `abort';
>>> libqemu-aarch64-linux-user.fa.p/linux-user_signal.c.o:/srv/_build/../../home/cvs/qemu/qemu/linux-user/signal.c:723: first defined here
>>
>> [PATCH v4 03/10] linux-user: Use die_with_signal with abort
>>
>> Sigh.
>>
>> I'd be double-cautious when overriding system functions like this, - it's
>> almost always a bad idea.
>
> Richard, I'm not sure, but with that change:
>
> -void abort(void)
> +void __attribute__((weak)) abort(void)
>
> it will at least successfully link the binary. Not sure which effects it has,
> but probably not worse than before your patch series...
This won't work, in that it will have no effect, and we continue to have the weird longjmp
assertion message after stack corruption.
Probably we will have to replace all of the apis that can raise abort at the source level,
e.g.
void qemu_abort(void) __attribute__((noreturn));
void qemu_abort_msg(const char *) __attribute__((noreturn));
#undef abort
#define abort qemu_abort
#undef assert
#define assert(X) ...
#undef g_assert
#define g_assert(X) assert(X)
etc.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-19 8:26 ` Michael Tokarev
@ 2023-09-19 8:38 ` Richard Henderson
2023-09-19 9:17 ` Helge Deller
2023-09-19 13:01 ` Michael Tokarev
0 siblings, 2 replies; 24+ messages in thread
From: Richard Henderson @ 2023-09-19 8:38 UTC (permalink / raw)
To: Michael Tokarev, Helge Deller, qemu-devel
On 9/19/23 10:26, Michael Tokarev wrote:
> 19.09.2023 11:00, Helge Deller wrote:
> ..
>>> Probably the right solution is to use qemu_abort() (and qemu_assert() etc),
>>> and maybe #define abort(x) qemu_abort(x). Even if some way to redefine
>>> abort like the above will work on glibc, it does not mean it will work
>>> on *bsd and in other contexts.
>>
>> True. That's probably the better solution.
>
> That wont work, since abort() gets called from a lot of libraries
> (gilbc has 1000s of calls to it)
>
> Sigh.
>
> /mjt
A possible solution that occurs to me is to treat SIGABRT like patch 7 of this patch set
treats SIGPROF: remap the guest signal to one of the host RT signals.
Then we leave the host SIGABRT as SIG_DFL, producing the expected crash when the signal
originates from a host abort() (etc). A guest abort() would use a different signal which
is caught and emulated.
Things do get confusing across processes, but should be no worse than any of the existing
signal number swizzling.
Thoughts?
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-19 8:38 ` Richard Henderson
@ 2023-09-19 9:17 ` Helge Deller
2023-09-19 13:01 ` Michael Tokarev
1 sibling, 0 replies; 24+ messages in thread
From: Helge Deller @ 2023-09-19 9:17 UTC (permalink / raw)
To: Richard Henderson, Michael Tokarev, qemu-devel
On 9/19/23 10:38, Richard Henderson wrote:
> On 9/19/23 10:26, Michael Tokarev wrote:
>> 19.09.2023 11:00, Helge Deller wrote:
>> ..
>>>> Probably the right solution is to use qemu_abort() (and qemu_assert() etc),
>>>> and maybe #define abort(x) qemu_abort(x). Even if some way to redefine
>>>> abort like the above will work on glibc, it does not mean it will work
>>>> on *bsd and in other contexts.
>>>
>>> True. That's probably the better solution.
>>
>> That wont work, since abort() gets called from a lot of libraries
>> (gilbc has 1000s of calls to it)
>>
>> Sigh.
>>
>> /mjt
>
> A possible solution that occurs to me is to treat SIGABRT like patch 7 of this patch set treats SIGPROF: remap the guest signal to one of the host RT signals.
>
> Then we leave the host SIGABRT as SIG_DFL, producing the expected crash when the signal originates from a host abort() (etc). A guest abort() would use a different signal which is caught and emulated.
>
> Things do get confusing across processes, but should be no worse than any of the existing signal number swizzling.
>
> Thoughts?
Yes, this could work.
Helge
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 00/10] linux-user: Detect and report host crashes
2023-09-19 8:38 ` Richard Henderson
2023-09-19 9:17 ` Helge Deller
@ 2023-09-19 13:01 ` Michael Tokarev
1 sibling, 0 replies; 24+ messages in thread
From: Michael Tokarev @ 2023-09-19 13:01 UTC (permalink / raw)
To: Richard Henderson, Helge Deller, qemu-devel
FWIW, there's an interesting read (suggested by iam_tj)
https://stackoverflow.com/questions/39725138/strange-behaviour-while-wrapping-abort-system-call
also https://drewdevault.com/2016/07/19/Using-Wl-wrap-for-mocking-in-C.html
/mjt
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-09-19 13:02 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-23 5:16 [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
2023-08-23 5:16 ` [PATCH v4 01/10] linux-user: Split out die_with_signal Richard Henderson
2023-08-23 12:55 ` Philippe Mathieu-Daudé
2023-08-23 5:16 ` [PATCH v4 02/10] linux-user: Exit not abort in die_with_backtrace Richard Henderson
2023-08-23 12:55 ` Philippe Mathieu-Daudé
2023-08-23 5:16 ` [PATCH v4 03/10] linux-user: Use die_with_signal with abort Richard Henderson
2023-08-23 5:16 ` [PATCH v4 04/10] linux-user: Detect and report host crashes Richard Henderson
2023-08-23 5:16 ` [PATCH v4 05/10] linux-user: Only register handlers for core_dump_signal by default Richard Henderson
2023-08-23 5:16 ` [PATCH v4 06/10] linux-user: Map unsupported signals to an out-of-bounds value Richard Henderson
2023-08-23 5:16 ` [PATCH v4 07/10] linux-user: Remap SIGPROF when CONFIG_GPROF Richard Henderson
2023-08-23 5:16 ` [PATCH v4 08/10] linux-user: Simplify signal_init Richard Henderson
2023-08-23 5:16 ` [PATCH v4 09/10] linux-user: Split out host_sig{segv,bus}_handler Richard Henderson
2023-08-23 5:16 ` [PATCH v4 10/10] linux-user: Detect and report host SIGILL, SIGFPE, SIGTRAP Richard Henderson
2023-09-09 19:12 ` [PATCH v4 00/10] linux-user: Detect and report host crashes Richard Henderson
2023-09-12 9:45 ` Helge Deller
2023-09-12 10:34 ` Michael Tokarev
2023-09-18 14:05 ` Helge Deller
2023-09-19 7:40 ` Michael Tokarev
2023-09-19 8:00 ` Helge Deller
2023-09-19 8:26 ` Michael Tokarev
2023-09-19 8:38 ` Richard Henderson
2023-09-19 9:17 ` Helge Deller
2023-09-19 13:01 ` Michael Tokarev
2023-09-19 8:29 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).