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

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

[Ilya Leoshkevich]

Based-on: <20230202005204.2055899-1-richard.henderson@linaro.org>
("[PATCH 00/14] linux-user/sparc: Handle missing traps")

v1: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg07251.html
v1 -> v2: Drop the sparc patch (superseded by Richard's series).
          Add the end_exclusive() fix.

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().

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

Best regards,
Ilya

Ilya Leoshkevich (4):
  linux-user: Always exit from exclusive state in fork_end()
  cpus: Make {start,end}_exclusive() recursive
  linux-user/microblaze: Handle privileged exception
  tests/tcg/linux-test: Add linux-fork-trap test

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

-- 
2.39.1

[PATCH v2 1/4] linux-user: Always exit from exclusive state in fork_end()

[Ilya Leoshkevich]

fork()ed processes currently start with
current_cpu->in_exclusive_context set, which is, strictly speaking, not
correct, but does not cause problems (even assertion failures).

With one of the next patches, the code begins to rely on this value, so
fix it by always calling end_exclusive() in fork_end().

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/main.c    | 10 ++++++----
 linux-user/syscall.c |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 4290651c3cf..4ff30ff9806 100644
--- 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 1e868e9b0e2..a6c426d73cf 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6752,6 +6752,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;
 }
-- 
2.39.1

Re: [PATCH v2 1/4] linux-user: Always exit from exclusive state in fork_end()

[Richard Henderson]

On 2/13/23 02:52, Ilya Leoshkevich wrote:
> fork()ed processes currently start with
> current_cpu->in_exclusive_context set, which is, strictly speaking, not
> correct, but does not cause problems (even assertion failures).
> 
> With one of the next patches, the code begins to rely on this value, so
> fix it by always calling end_exclusive() in fork_end().
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   linux-user/main.c    | 10 ++++++----
>   linux-user/syscall.c |  1 +
>   2 files changed, 7 insertions(+), 4 deletions(-)

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

r~

Re: [PATCH v2 1/4] linux-user: Always exit from exclusive state in fork_end()

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> fork()ed processes currently start with
> current_cpu->in_exclusive_context set, which is, strictly speaking, not
> correct, but does not cause problems (even assertion failures).
>
> With one of the next patches, the code begins to rely on this value, so
> fix it by always calling end_exclusive() in fork_end().
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 2/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 v2 2/4] cpus: Make {start,end}_exclusive() recursive

[Richard Henderson]

On 2/13/23 02:52, 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>
> ---
>   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++;

I think this line would be clearer as "= 1".

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


r~

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

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> 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>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 3/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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 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

[PATCH v2 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 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 v2 4/4] tests/tcg/linux-test: Add linux-fork-trap test

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> Check that dying due to a signal does not deadlock.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  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. */

I think we could benefit from two printfs in the test so the following:

  ➜  ./qemu-aarch64 ./tests/tcg/aarch64-linux-user/linux-fork-trap 
  qemu: uncaught target signal 5 (Trace/breakpoint trap) - core dumped
  🕙09:50:14 alex@zen:qemu.git/builds/arm.all  on  testing/next [$!?] 
  ➜  echo $status
  0

is a little less confusing.

> +        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.
> +         */

 printf("about to trigger fault...\n");

> +#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));
> +

  printf("faulting thread exited cleanly\n");

Othwerwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>



> +    return EXIT_SUCCESS;
> +}


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro