[PATCH 0/4] Fix deadlock when dying because of a signal

[PATCH 0/4] Fix deadlock when dying because of a signal

[Ilya Leoshkevich]

Hi,

wasmtime testsuite found a deadlock in qemu_plugin_user_exit().
I tracked it down to one of my earlier patches, which introduced
cleanup in dump_core_and_abort().

Patch 1 fixes the issue, patches 2 and 3 fix __builtin_trap()
handling in microblaze and sparc - which is needed for patch 4,
that adds a test.

Just before sending this, I noticed that a solution has already been
proposed in [1], but apparently it wasn't accepted.

Best regards,
Ilya

[1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg03506.html

Ilya Leoshkevich (4):
  cpus: Make {start,end}_exclusive() recursive
  linux-user/microblaze: Handle privileged exception
  linux-user/sparc: Handle "ta 5"
  tests/tcg/linux-test: Add linux-fork-trap test

 cpus-common.c                               | 12 +++++-
 include/hw/core/cpu.h                       |  4 +-
 linux-user/microblaze/cpu_loop.c            | 10 ++++-
 linux-user/sparc/cpu_loop.c                 |  3 ++
 tests/tcg/multiarch/linux/linux-fork-trap.c | 48 +++++++++++++++++++++
 5 files changed, 71 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/multiarch/linux/linux-fork-trap.c

-- 
2.39.1

[PATCH 1/4] cpus: Make {start,end}_exclusive() recursive

[Ilya Leoshkevich]

Currently dying to one of the core_dump_signal()s deadlocks, because
dump_core_and_abort() calls start_exclusive() two times: first via
stop_all_tasks(), and then via preexit_cleanup() ->
qemu_plugin_user_exit().

There are a number of ways to solve this: resume after dumping core;
check cpu_in_exclusive_context() in qemu_plugin_user_exit(); or make
{start,end}_exclusive() recursive. Pick the last option, since it's
the most straightforward one.

Fixes: da91c1920242 ("linux-user: Clean up when exiting due to a signal")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 cpus-common.c         | 12 ++++++++++--
 include/hw/core/cpu.h |  4 ++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/cpus-common.c b/cpus-common.c
index 793364dc0ed..a0c52cd187f 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -192,6 +192,11 @@ void start_exclusive(void)
     CPUState *other_cpu;
     int running_cpus;
 
+    if (current_cpu->exclusive_context_count) {
+        current_cpu->exclusive_context_count++;
+        return;
+    }
+
     qemu_mutex_lock(&qemu_cpu_list_lock);
     exclusive_idle();
 
@@ -219,13 +224,16 @@ void start_exclusive(void)
      */
     qemu_mutex_unlock(&qemu_cpu_list_lock);
 
-    current_cpu->in_exclusive_context = true;
+    current_cpu->exclusive_context_count++;
 }
 
 /* Finish an exclusive operation.  */
 void end_exclusive(void)
 {
-    current_cpu->in_exclusive_context = false;
+    current_cpu->exclusive_context_count--;
+    if (current_cpu->exclusive_context_count) {
+        return;
+    }
 
     qemu_mutex_lock(&qemu_cpu_list_lock);
     qatomic_set(&pending_cpus, 0);
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 2417597236b..671f041bec6 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -349,7 +349,7 @@ struct CPUState {
     bool unplug;
     bool crash_occurred;
     bool exit_request;
-    bool in_exclusive_context;
+    int exclusive_context_count;
     uint32_t cflags_next_tb;
     /* updates protected by BQL */
     uint32_t interrupt_request;
@@ -758,7 +758,7 @@ void async_safe_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data
  */
 static inline bool cpu_in_exclusive_context(const CPUState *cpu)
 {
-    return cpu->in_exclusive_context;
+    return cpu->exclusive_context_count;
 }
 
 /**
-- 
2.39.1

Re: [PATCH 1/4] cpus: Make {start,end}_exclusive() recursive

[Ilya Leoshkevich]

On Wed, 2023-02-01 at 01:46 +0100, Ilya Leoshkevich wrote:
> Currently dying to one of the core_dump_signal()s deadlocks, because
> dump_core_and_abort() calls start_exclusive() two times: first via
> stop_all_tasks(), and then via preexit_cleanup() ->
> qemu_plugin_user_exit().
> 
> There are a number of ways to solve this: resume after dumping core;
> check cpu_in_exclusive_context() in qemu_plugin_user_exit(); or make
> {start,end}_exclusive() recursive. Pick the last option, since it's
> the most straightforward one.
> 
> Fixes: da91c1920242 ("linux-user: Clean up when exiting due to a
> signal")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Hi,

I noticed that fork()ed CPUs start with in_exclusive_context set (in
this patch it is renamed to exclusive_context_count, but the point
stands). That was not important before, since only pending_cpus decided
what happens in start_exclusive()/end_exclusive(). Now that
exclusive_context_count is also important, we need something like:

--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -161,13 +161,15 @@ void fork_end(int child)
         }
         qemu_init_cpu_list();
         gdbserver_fork(thread_cpu);
-        /* qemu_init_cpu_list() takes care of reinitializing the
-         * exclusive state, so we don't need to end_exclusive() here.
-         */
     } else {
         cpu_list_unlock();
-        end_exclusive();
     }
+    /*
+     * qemu_init_cpu_list() reinitialized the child exclusive state,
but we
+     * also need to keep current_cpu consistent, so call
end_exclusive() for
+     * both child and parent.
+     */
+    end_exclusive();
 }
 
 __thread CPUState *thread_cpu;
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1f8c10f8ef9..70fad4bed01 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6776,6 +6776,7 @@ static int do_fork(CPUArchState *env, unsigned
int flags, abi_ulong newsp,
             cpu_clone_regs_parent(env, flags);
             fork_end(0);
         }
+        g_assert(!cpu_in_exclusive_context(cpu));
     }
     return ret;
 }

I can include this in v2, if the overall recursive lock approach is
considered appropriate.

Best regards,
Ilya

Re: [PATCH 1/4] cpus: Make {start,end}_exclusive() recursive

[Richard Henderson]

On 2/1/23 03:07, Ilya Leoshkevich wrote:
> I can include this in v2, if the overall recursive lock approach is
> considered appropriate.

I think it's a good idea.  Certainly an improvement over removing the lock from the plugin 
code, like I tried.


r~

[PATCH 2/4] linux-user/microblaze: Handle privileged exception

[Ilya Leoshkevich]

Follow what kernel's full_exception() is doing.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/microblaze/cpu_loop.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/linux-user/microblaze/cpu_loop.c b/linux-user/microblaze/cpu_loop.c
index 5ccf9e942ea..212e62d0a62 100644
--- a/linux-user/microblaze/cpu_loop.c
+++ b/linux-user/microblaze/cpu_loop.c
@@ -25,8 +25,8 @@
 
 void cpu_loop(CPUMBState *env)
 {
+    int trapnr, ret, si_code, sig;
     CPUState *cs = env_cpu(env);
-    int trapnr, ret, si_code;
 
     while (1) {
         cpu_exec_start(cs);
@@ -76,6 +76,7 @@ void cpu_loop(CPUMBState *env)
             env->iflags &= ~(IMM_FLAG | D_FLAG);
             switch (env->esr & 31) {
             case ESR_EC_DIVZERO:
+                sig = TARGET_SIGFPE;
                 si_code = TARGET_FPE_INTDIV;
                 break;
             case ESR_EC_FPU:
@@ -84,6 +85,7 @@ void cpu_loop(CPUMBState *env)
                  * if there's no recognized bit set.  Possibly this
                  * implies that si_code is 0, but follow the structure.
                  */
+                sig = TARGET_SIGFPE;
                 si_code = env->fsr;
                 if (si_code & FSR_IO) {
                     si_code = TARGET_FPE_FLTINV;
@@ -97,13 +99,17 @@ void cpu_loop(CPUMBState *env)
                     si_code = TARGET_FPE_FLTRES;
                 }
                 break;
+            case ESR_EC_PRIVINSN:
+                sig = SIGILL;
+                si_code = ILL_PRVOPC;
+                break;
             default:
                 fprintf(stderr, "Unhandled hw-exception: 0x%x\n",
                         env->esr & ESR_EC_MASK);
                 cpu_dump_state(cs, stderr, 0);
                 exit(EXIT_FAILURE);
             }
-            force_sig_fault(TARGET_SIGFPE, si_code, env->pc);
+            force_sig_fault(sig, si_code, env->pc);
             break;
 
         case EXCP_DEBUG:
-- 
2.39.1

Re: [PATCH 2/4] linux-user/microblaze: Handle privileged exception

[Richard Henderson]

On 1/31/23 14:46, Ilya Leoshkevich wrote:
> Follow what kernel's full_exception() is doing.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   linux-user/microblaze/cpu_loop.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/4] linux-user/sparc: Handle "ta 5"

[Ilya Leoshkevich]

GCC lowers __builtin_trap() to "ta 5", which in turn generates trap
0x105. Follow what kernel's bad_trap() is doing there.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/sparc/cpu_loop.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/linux-user/sparc/cpu_loop.c b/linux-user/sparc/cpu_loop.c
index 434c90a55f8..fa36d452a51 100644
--- a/linux-user/sparc/cpu_loop.c
+++ b/linux-user/sparc/cpu_loop.c
@@ -225,6 +225,9 @@ void cpu_loop (CPUSPARCState *env)
             restore_window(env);
             break;
 #ifndef TARGET_ABI32
+        case 0x105:
+            force_sig_fault(TARGET_SIGILL, ILL_ILLTRP, env->pc);
+            break;
         case 0x16e:
             flush_windows(env);
             sparc64_get_context(env);
-- 
2.39.1

Re: [PATCH 3/4] linux-user/sparc: Handle "ta 5"

[Richard Henderson]

On 1/31/23 14:46, Ilya Leoshkevich wrote:
> GCC lowers __builtin_trap() to "ta 5", which in turn generates trap
> 0x105. Follow what kernel's bad_trap() is doing there.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/sparc/cpu_loop.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/linux-user/sparc/cpu_loop.c b/linux-user/sparc/cpu_loop.c
> index 434c90a55f8..fa36d452a51 100644
> --- a/linux-user/sparc/cpu_loop.c
> +++ b/linux-user/sparc/cpu_loop.c
> @@ -225,6 +225,9 @@ void cpu_loop (CPUSPARCState *env)
>               restore_window(env);
>               break;
>   #ifndef TARGET_ABI32
> +        case 0x105:
> +            force_sig_fault(TARGET_SIGILL, ILL_ILLTRP, env->pc);
> +            break;
>           case 0x16e:
>               flush_windows(env);
>               sparc64_get_context(env);

Quite a lot of ttable{32,64}.S is missing here.  We should certainly make use of TT_TRAP, 
instead of hard-coding 0x100 vs 0x80.


r~

[PATCH 4/4] tests/tcg/linux-test: Add linux-fork-trap test

[Ilya Leoshkevich]

Check that dying due to a signal does not deadlock.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/multiarch/linux/linux-fork-trap.c | 48 +++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 tests/tcg/multiarch/linux/linux-fork-trap.c

diff --git a/tests/tcg/multiarch/linux/linux-fork-trap.c b/tests/tcg/multiarch/linux/linux-fork-trap.c
new file mode 100644
index 00000000000..a921f875380
--- /dev/null
+++ b/tests/tcg/multiarch/linux/linux-fork-trap.c
@@ -0,0 +1,48 @@
+/*
+ * Test that a fork()ed process terminates after __builtin_trap().
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <stdlib.h>
+#include <sys/resource.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+int main(void)
+{
+    struct rlimit nodump;
+    pid_t err, pid;
+    int wstatus;
+
+    pid = fork();
+    assert(pid != -1);
+    if (pid == 0) {
+        /* We are about to crash on purpose; disable core dumps. */
+        if (getrlimit(RLIMIT_CORE, &nodump)) {
+            return EXIT_FAILURE;
+        }
+        nodump.rlim_cur = 0;
+        if (setrlimit(RLIMIT_CORE, &nodump)) {
+            return EXIT_FAILURE;
+        }
+        /*
+         * An alternative would be to dereference a NULL pointer, but that
+         * would be an UB in C.
+         */
+#if defined(__MICROBLAZE__)
+        /*
+         * gcc emits "bri 0", which is an endless loop.
+         * Take glibc's ABORT_INSTRUCTION.
+         */
+        asm volatile("brki r0,-1");
+#else
+        __builtin_trap();
+#endif
+    }
+    err = waitpid(pid, &wstatus, 0);
+    assert(err == pid);
+    assert(WIFSIGNALED(wstatus));
+
+    return EXIT_SUCCESS;
+}
-- 
2.39.1

Re: [PATCH 4/4] tests/tcg/linux-test: Add linux-fork-trap test

[Richard Henderson]

On 1/31/23 14:46, Ilya Leoshkevich wrote:
> Check that dying due to a signal does not deadlock.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   tests/tcg/multiarch/linux/linux-fork-trap.c | 48 +++++++++++++++++++++
>   1 file changed, 48 insertions(+)
>   create mode 100644 tests/tcg/multiarch/linux/linux-fork-trap.c

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~