linux-kernel - Re: strace lockup when tracing exec in go

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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

On Thu 22-09-16 06:15:02, Mike Galbraith wrote:
[...]
> master.today...

Thanks for trying to reproduce this. My tiny laptop (2 cores, 2 threads
per core) cannot reproduce even in 10 minutes or so. I've tried to use
the same machine I was testing with 3.12 kernel (2 sockets, 8 cores per
soc. and 2 threas per core) and it hit almost instantly. I have tried 
mutex_lock_killable -> interruptible and it didn't help as I've
expected. So the current kernel doesn't do any magic to prevent from the
issue as well.

So I've stared into do_notify_parent some more and the following was
just very confusing

	if (!tsk->ptrace && sig == SIGCHLD &&
	    (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
	     (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) {
		/*
		 * We are exiting and our parent doesn't care.  POSIX.1
		 * defines special semantics for setting SIGCHLD to SIG_IGN
		 * or setting the SA_NOCLDWAIT flag: we should be reaped
		 * automatically and not left for our parent's wait4 call.
		 * Rather than having the parent do it as a magic kind of
		 * signal handler, we just set this to tell do_exit that we
		 * can be cleaned up without becoming a zombie.  Note that
		 * we still call __wake_up_parent in this case, because a
		 * blocked sys_wait4 might now return -ECHILD.
		 *
		 * Whether we send SIGCHLD or not for SA_NOCLDWAIT
		 * is implementation-defined: we do (if you don't want
		 * it, just use SIG_IGN instead).
		 */
		autoreap = true;
		if (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN)
			sig = 0;
	}

it tries to prevent from what I am seeing in a way. If the SIGCHLD is
ignored then it just does autoreap and everything is fine. But this
doesn't seem to be the case here. In fact we are not sending the signal
because sig_task_ignored is true resp. sig_handler_ignored which can
fail even for handler == SIG_DFL && sig_kernel_ignore() and SIGCHLD
seems to be in SIG_KERNEL_IGNORE_MASK. So I've tried

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..1840c7f4e3c2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1630,7 +1630,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
 	psig = tsk->parent->sighand;
 	spin_lock_irqsave(&psig->siglock, flags);
 	if (!tsk->ptrace && sig == SIGCHLD &&
-	    (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
+	    (sig_handler_ignored(&psig->action[SIGCHLD-1].sa.sa_handler, SIGCHLD) ||
 	     (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) {
 		/*
 		 * We are exiting and our parent doesn't care.  POSIX.1

And it worked out. Now I do not have any idea why the check is explicit
for SIG_IGN rather than sig_handler_ignored, maybe there is a strong
reason for that. Oleg? I will cook up a full patch if this looks ok.
-- 
Michal Hocko
SUSE Labs