[Qemu-devel] [PATCH] linux-user: Fix locking order in fork_start()

[Qemu-devel] [PATCH] linux-user: Fix locking order in fork_start()

[Peter Maydell]

Our locking order is that the tb lock should be taken
inside the mmap_lock, but fork_start() grabs locks the
other way around. This means that if a heavily multithreaded
guest process (such as Java) calls fork() it can deadlock,
with the thread that called fork() stuck in fork_start()
with the tb lock and waiting for the mmap lock, but some
other thread in tb_find() with the mmap lock and waiting
for the tb lock. The cpu_list_lock() should also always be
taken last, not first.

Fix this by making fork_start() grab the locks in the
right order. The order in which we drop locks doesn't
matter, so we leave fork_end() the way it is.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-stable@nongnu.org
---
 linux-user/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 6286661..146ee3e 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -128,9 +128,9 @@ int cpu_get_pic_interrupt(CPUX86State *env)
 /* Make sure everything is in a consistent state for calling fork().  */
 void fork_start(void)
 {
-    cpu_list_lock();
-    qemu_mutex_lock(&tb_ctx.tb_lock);
     mmap_fork_start();
+    qemu_mutex_lock(&tb_ctx.tb_lock);
+    cpu_list_lock();
 }
 
 void fork_end(int child)
-- 
2.7.4

Re: [Qemu-devel] [PATCH] linux-user: Fix locking order in fork_start()

[Paolo Bonzini]

On 04/12/2017 15:22, Peter Maydell wrote:
> Our locking order is that the tb lock should be taken
> inside the mmap_lock, but fork_start() grabs locks the
> other way around. This means that if a heavily multithreaded
> guest process (such as Java) calls fork() it can deadlock,
> with the thread that called fork() stuck in fork_start()
> with the tb lock and waiting for the mmap lock, but some
> other thread in tb_find() with the mmap lock and waiting
> for the tb lock. The cpu_list_lock() should also always be
> taken last, not first.
> 
> Fix this by making fork_start() grab the locks in the
> right order. The order in which we drop locks doesn't
> matter, so we leave fork_end() the way it is.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>  linux-user/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 6286661..146ee3e 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -128,9 +128,9 @@ int cpu_get_pic_interrupt(CPUX86State *env)
>  /* Make sure everything is in a consistent state for calling fork().  */
>  void fork_start(void)
>  {
> -    cpu_list_lock();
> -    qemu_mutex_lock(&tb_ctx.tb_lock);
>      mmap_fork_start();
> +    qemu_mutex_lock(&tb_ctx.tb_lock);
> +    cpu_list_lock();
>  }
>  
>  void fork_end(int child)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] linux-user: Fix locking order in fork_start()

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Our locking order is that the tb lock should be taken
> inside the mmap_lock, but fork_start() grabs locks the
> other way around. This means that if a heavily multithreaded
> guest process (such as Java) calls fork() it can deadlock,
> with the thread that called fork() stuck in fork_start()
> with the tb lock and waiting for the mmap lock, but some
> other thread in tb_find() with the mmap lock and waiting
> for the tb lock. The cpu_list_lock() should also always be
> taken last, not first.
>
> Fix this by making fork_start() grab the locks in the
> right order. The order in which we drop locks doesn't
> matter, so we leave fork_end() the way it is.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>  linux-user/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 6286661..146ee3e 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -128,9 +128,9 @@ int cpu_get_pic_interrupt(CPUX86State *env)
>  /* Make sure everything is in a consistent state for calling fork().  */
>  void fork_start(void)
>  {
> -    cpu_list_lock();
> -    qemu_mutex_lock(&tb_ctx.tb_lock);
>      mmap_fork_start();
> +    qemu_mutex_lock(&tb_ctx.tb_lock);
> +    cpu_list_lock();
>  }
>
>  void fork_end(int child)

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

--
Alex Bennée

Re: [Qemu-devel] [PATCH] linux-user: Fix locking order in fork_start()

[Laurent Vivier]

Le 04/12/2017 à 15:22, Peter Maydell a écrit :
> Our locking order is that the tb lock should be taken
> inside the mmap_lock, but fork_start() grabs locks the
> other way around. This means that if a heavily multithreaded
> guest process (such as Java) calls fork() it can deadlock,
> with the thread that called fork() stuck in fork_start()
> with the tb lock and waiting for the mmap lock, but some
> other thread in tb_find() with the mmap lock and waiting
> for the tb lock. The cpu_list_lock() should also always be
> taken last, not first.
> 
> Fix this by making fork_start() grab the locks in the
> right order. The order in which we drop locks doesn't
> matter, so we leave fork_end() the way it is.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>  linux-user/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 6286661..146ee3e 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -128,9 +128,9 @@ int cpu_get_pic_interrupt(CPUX86State *env)
>  /* Make sure everything is in a consistent state for calling fork().  */
>  void fork_start(void)
>  {
> -    cpu_list_lock();
> -    qemu_mutex_lock(&tb_ctx.tb_lock);
>      mmap_fork_start();
> +    qemu_mutex_lock(&tb_ctx.tb_lock);
> +    cpu_list_lock();
>  }
>  
>  void fork_end(int child)
> 

Applied to my linux-user branch.

Thanks,
Laurent