Date:   Thu, 22 Sep 2016 15:53:01 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        strace-devel@...ts.sourceforge.net,
        Mike Galbraith <umgwanakikbuti@...il.com>,
        Aleksa Sarai <asarai@...e.com>
Subject: Re: strace lockup when tracing exec in go

On Thu 22-09-16 13:09:25, Michal Hocko wrote:
> On Thu 22-09-16 12:09:05, Mike Galbraith wrote:
> > On Thu, 2016-09-22 at 11:53 +0200, Michal Hocko wrote:
> > > On Thu 22-09-16 11:40:09, Mike Galbraith wrote:
> > 
> > > > This patch doesn't help, nor does the previous patch... but with both
> > > > applied, all is well.  All you have to do now is figure out why :)
> > > 
> > > Ohh, I should be more explicit, this needs the mm_access part as well.
> > > Sorry for not being clear enough. So the full change is
> > 
> > Ah.  That was gonna happen after lunch, but since you already know it
> > works, I can get back to un-b0rking one of my trees.
> 
> Yeah, it should work. The testcase is running in a loop for more than
> hour already and everything seems to be ok. Now the question is whether
> the fix is really correct which is something for Oleg.
> 
> Also I think there is at least one more issue lurking there. Without or
> without the patch I can make tracer get stuck in do_wait waiting for
> task which doesn't seem to be alive anymore. I will report it separately
> after this one gets resolved to not pull more confusion in.

OK, the test in the loop has survived 3h of runtime without a single
lockup so the patch seems to be working for this case. I am posting the
patch with the full changelog, let's see if it is correct as well. As
I've said earlier this might be a strace bug which does an unexpected
syscall while it should be doing wait on the child process instead.

If the patch is correct then I would mark it for stable as well.
---
>From fe82d463fd2ef1585d2c37bf9fa6a1761e6ee0e5 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@...e.com>
Date: Thu, 22 Sep 2016 10:09:34 +0200
Subject: [PATCH] signal: always signal tracer from the ptraced task

Aleksa has reported the following lockup when stracing the following go
program

% cat exec.go
package main

import (
    "os"
    "syscall"
)

func main() {
    syscall.Exec("/bin/echo", []string{"/bin/echo", "Hello"}, os.Environ())
}
$ go version
go version go1.6.3 linux/amd64
$ go build -o exec exec.go

$ strace -f ./exec
[snip]
[pid 10349] select(0, NULL, NULL, NULL, {0, 100} <unfinished ...>
[pid 10346] <... select resumed> )      = 0 (Timeout)
[pid 10346] select(0, NULL, NULL, NULL, {0, 20} <unfinished ...>
[pid 10345] execve("/bin/echo", ["/bin/echo", "Hello"], [/* 95 vars */] <unfinished ...>
[pid 10346] <... select resumed> )      = 0 (Timeout)
[pid 10349] <... select resumed> )      = 0 (Timeout)

execve will never finish unless the strace process get killed with
SIGKILL. The reason is the following deadlock

tracer				thread_A				thread_$N
SyS_process_vm_readv		SyS_execve				do_exit
				  do_execveat_common			  exit_notify
				    prepare_bprm_creds			    do_notify_parent
				      mutex_lock(cred_guard_mutex)	      __group_send_sig_info
				    search_binary_handler		        send_signal
				      load_elf_binary			          prepare_signal -> fail SIGHCHLD is SGL_DFL
				        flush_old_exec
				          # wait for sig->notify_count
  process_vm_rw
    process_vm_rw_core
      mm_access
        mutex_lock_killable(cred_guard_mutex)

So there seems to be 2 issues here. The first one is that exiting
threads (because of the ongoing exec) are not sending SIGCHLD signal
to the tracer but they rely on the tracer to reap them and call
release_task->__exit_signal which in turn would wake up the thread_A and
move on with the exec. The other part of the story is that the tracer
is not in do_wait but rather calls process_vm_readv (presumably to get
arguments of the syscall) and it waits for a lock in killable rather
than interruptible sleep.

The fix is therefore twofold. We want to teach mm_access to sleep in
interruptible sleep and we want to make sure that the traced child
will send the signal to the parent even when it is ignored or SIG_DFL.
sig_ignored already seems to be doing something along that line except
it doesn't handle when a traced child sends a signal to the tracer.
Fix this by checking the current ptrace status and whether the target
task is the tracer.

Reported-by: Aleksa Sarai <asarai@...e.com>
Signed-off-by: Michal Hocko <mhocko@...e.com>
---
 kernel/fork.c   | 2 +-
 kernel/signal.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 5a57b9bab85c..d5b7c3aea187 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -837,7 +837,7 @@ struct mm_struct *mm_access(struct task_struct *task, unsigned int mode)
 	struct mm_struct *mm;
 	int err;
 
-	err =  mutex_lock_killable(&task->signal->cred_guard_mutex);
+	err =  mutex_lock_interruptible(&task->signal->cred_guard_mutex);
 	if (err)
 		return ERR_PTR(err);
 
diff --git a/kernel/signal.c b/kernel/signal.c
index 96e9bc40667f..5c8b84b76f0b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -91,6 +91,10 @@ static int sig_ignored(struct task_struct *t, int sig, bool force)
 	if (!sig_task_ignored(t, sig, force))
 		return 0;
 
+	/* Do not ignore signals sent from child to the parent */
+	if (current->ptrace && current->parent == t)
+		return 0;
+
 	/*
 	 * Tracers may want to know about even ignored signals.
 	 */
-- 
2.9.3

-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists