Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1591596 > unrolled thread

Re: [PATCH 0/2] fix the traced mt-exec deadlock

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/2] fix the traced mt-exec deadlock ebiederm@xmission.com (Eric W. Biederman) - 2017-03-03 02:20 +0100
    Re: [PATCH 0/2] fix the traced mt-exec deadlock Oleg Nesterov <oleg@redhat.com> - 2017-03-03 18:40 +0100
      Re: [PATCH 0/2] fix the traced mt-exec deadlock ebiederm@xmission.com (Eric W. Biederman) - 2017-03-03 20:30 +0100
        [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped. ebiederm@xmission.com (Eric W. Biederman) - 2017-03-03 21:20 +0100
          Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped. Oleg Nesterov <oleg@redhat.com> - 2017-03-04 18:10 +0100
        Re: [PATCH 0/2] fix the traced mt-exec deadlock ebiederm@xmission.com (Eric W. Biederman) - 2017-03-03 22:40 +0100
        Re: [PATCH 0/2] fix the traced mt-exec deadlock ebiederm@xmission.com (Eric W. Biederman) - 2017-03-04 01:10 +0100
        Re: [PATCH 0/2] fix the traced mt-exec deadlock Oleg Nesterov <oleg@redhat.com> - 2017-03-04 18:10 +0100

#1591596 — Re: [PATCH 0/2] fix the traced mt-exec deadlock

Oleg Nesterov <oleg@redhat.com> writes:

> Eric,
>
> our discussion was a bit confusing, and it seems that we did not
> fully convince each other. So let me ask what do you finally think
> about this fix.
>
> Let me repeat. Even if I do not agree with some of your objections,
> I do agree that 1/2 does not look nice and clean. And we seem to
> agree that either way, with or without this fix, we need more changes
> in this area.
>
> But we need a simple and backportable fix for stable trees, say for
> rhel7. This bug was reported many times, and this is the simplest
> solution I was able to find.

I am not 100% convinced that we need a backportable solution, but
regardless my experience is that good clean solutions are almost always
easier to backport that something we just settle for.

The patch below needs a little more looking and testing but arguably
it is sufficient.

It implements autoreaping for non-leader threads always.  We might want
to limit this to the case of exec.  For exec there is a strong argument
that no one cares as the exit_code/status returned by these exiting
threads has always been 0.  Maybe that is ok.  Exit_success but it
certainly seems wrong in the case of exec.  It would really be better to
have an exit status that you went away with exec.

I know it has always been 0 as fs/exec.c does not set
sig->group_exit_code so the value initialized in copy_signal from
kmem_cache_zalloc is used.  AKA 0.

This change has very little to do with the cred_guard_mutex.  Just a
question of which semantics we want to export.

In my preliminary testing this seems a viable.

My big problem with your previous patch is that it is in no way
obviously correct.  Nor is it well explained.  All signs that someone is
pushing too hard and something better can be achieved if someone steps
back a little.

When the following patch can be made to solve the same problem we
certainly need an explanation in the commit comment on why we are using
much more code for the same job.

diff --git a/kernel/exit.c b/kernel/exit.c
index 5cfbd595f918..5a432e0dcf4a 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -690,7 +690,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 				thread_group_empty(tsk) &&
 				!ptrace_reparented(tsk) ?
 			tsk->exit_signal : SIGCHLD;
-		autoreap = do_notify_parent(tsk, sig);
+		do_notify_parent(tsk, sig);
+		/* Autoreap threads even when ptraced */
+		autoreap = !thread_group_leader(tsk);
 	} else if (thread_group_leader(tsk)) {
 		autoreap = thread_group_empty(tsk) &&
 			do_notify_parent(tsk, tsk->exit_signal);
@@ -699,8 +701,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	}
 
 	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;
-	if (tsk->exit_state == EXIT_DEAD)
-		list_add(&tsk->ptrace_entry, &dead);
 
 	/* mt-exec, de_thread() is waiting for group leader */
 	if (unlikely(tsk->signal->notify_count < 0))
@@ -711,6 +711,8 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 		list_del_init(&p->ptrace_entry);
 		release_task(p);
 	}
+	if (autoreap)
+		release_task(tsk);
 }
 
 #ifdef CONFIG_DEBUG_STACK_USAGE

Eric

[toc] | [next] | [standalone]

#1592167

On 03/02, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > our discussion was a bit confusing, and it seems that we did not
> > fully convince each other. So let me ask what do you finally think
> > about this fix.
> >
> > Let me repeat. Even if I do not agree with some of your objections,
> > I do agree that 1/2 does not look nice and clean. And we seem to
> > agree that either way, with or without this fix, we need more changes
> > in this area.
> >
> > But we need a simple and backportable fix for stable trees, say for
> > rhel7. This bug was reported many times, and this is the simplest
> > solution I was able to find.
>
> I am not 100% convinced that we need a backportable solution,

I think we need, this was already requested,

> but
> regardless my experience is that good clean solutions are almost always
> easier to backport that something we just settle for.

Sure.

> The patch below needs a little more looking and testing but arguably
> it is sufficient.
>
> It implements autoreaping for non-leader threads always.  We might want
> to limit this to the case of exec.

I should have mentioned this. Of course, this change was proposed from the
very beginning, when this problem was reported first time. And of course
I like this fix much more than my patch (but yes, we shouldd limit it to
the case of exec).

The only problem is that it is incompatible change, and I have no idea what
can be broken.

Trivial test-case:

	void *thread(void *arg)
	{
		for (;;)
			pause();
		return NULL;
	}

	int main(void)
	{
		pthread_t pt;
		pthread_create(&pt, NULL, thread, NULL);
		execlp("true", "true", NULL);
	}

with your patch applied

	$ strace -f ./test
	strace: wait4(__WALL): No child processes

Yes, this is just a warning, but still. Now we need to change strace. And we
can't know what else can be confused/broken by this change.

man(ptrace) even documents that all other threads except the thread group leader
report death as if they exited via _exit(2).

Yes, yes, it also says that other threads stop in PTRACE_EVENT_EXIT stop,
so 2/2 (which we need with your change too) is is not compatible too and
I am worried, but:

	- this was never really true, an already exiting thread won't stop
	  if it races with exec

	- PTRACE_O_TRACEEXEC is not used that often, it never really worked

	- man(ptrace) also mentions that PTRACE_EVENT_EXIT behaviour may be
	  change in the future.

In short. Of course I considered this change. Looks too risky to me. But.
I will be happy if you send this change and take all the blame ;) I won't
argue (if you limit it to execve case).



> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -690,7 +690,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>  				thread_group_empty(tsk) &&
>  				!ptrace_reparented(tsk) ?
>  			tsk->exit_signal : SIGCHLD;
> -		autoreap = do_notify_parent(tsk, sig);
> +		do_notify_parent(tsk, sig);
> +		/* Autoreap threads even when ptraced */
> +		autoreap = !thread_group_leader(tsk);
>  	} else if (thread_group_leader(tsk)) {
>  		autoreap = thread_group_empty(tsk) &&
>  			do_notify_parent(tsk, tsk->exit_signal);

This is all you need,

> @@ -699,8 +701,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>  	}
>
>  	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;
> -	if (tsk->exit_state == EXIT_DEAD)
> -		list_add(&tsk->ptrace_entry, &dead);
>
>  	/* mt-exec, de_thread() is waiting for group leader */
>  	if (unlikely(tsk->signal->notify_count < 0))
> @@ -711,6 +711,8 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>  		list_del_init(&p->ptrace_entry);
>  		release_task(p);
>  	}
> +	if (autoreap)
> +		release_task(tsk);

These 2 changes are not needed. release_task(tsk) will be called by
list_for_each_entry_safe() above if autoreap == T.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1592240

Cc'd linux-api as we are talking about a deliberate user visible API
change here.

Oleg Nesterov <oleg@redhat.com> writes:

> On 03/02, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > our discussion was a bit confusing, and it seems that we did not
>> > fully convince each other. So let me ask what do you finally think
>> > about this fix.
>> >
>> > Let me repeat. Even if I do not agree with some of your objections,
>> > I do agree that 1/2 does not look nice and clean. And we seem to
>> > agree that either way, with or without this fix, we need more changes
>> > in this area.
>> >
>> > But we need a simple and backportable fix for stable trees, say for
>> > rhel7. This bug was reported many times, and this is the simplest
>> > solution I was able to find.
>>
>> I am not 100% convinced that we need a backportable solution,
>
> I think we need, this was already requested,
>
>> but
>> regardless my experience is that good clean solutions are almost always
>> easier to backport that something we just settle for.
>
> Sure.
>
>> The patch below needs a little more looking and testing but arguably
>> it is sufficient.
>>
>> It implements autoreaping for non-leader threads always.  We might want
>> to limit this to the case of exec.
>
> I should have mentioned this. Of course, this change was proposed from the
> very beginning, when this problem was reported first time. And of course
> I like this fix much more than my patch (but yes, we shouldd limit it to
> the case of exec).
>
> The only problem is that it is incompatible change, and I have no idea what
> can be broken.
>
> Trivial test-case:
>
> 	void *thread(void *arg)
> 	{
> 		for (;;)
> 			pause();
> 		return NULL;
> 	}
>
> 	int main(void)
> 	{
> 		pthread_t pt;
> 		pthread_create(&pt, NULL, thread, NULL);
> 		execlp("true", "true", NULL);
> 	}
>
> with your patch applied
>
> 	$ strace -f ./test
> 	strace: wait4(__WALL): No child processes
>
> Yes, this is just a warning, but still. Now we need to change strace. And we
> can't know what else can be confused/broken by this change.
>
> man(ptrace) even documents that all other threads except the thread group leader
> report death as if they exited via _exit(2).
>
> Yes, yes, it also says that other threads stop in PTRACE_EVENT_EXIT stop,
> so 2/2 (which we need with your change too) is is not compatible too and
> I am worried, but:
>
> 	- this was never really true, an already exiting thread won't stop
> 	  if it races with exec
>
> 	- PTRACE_O_TRACEEXEC is not used that often, it never really worked
>
> 	- man(ptrace) also mentions that PTRACE_EVENT_EXIT behaviour may be
> 	  change in the future.
>
> In short. Of course I considered this change. Looks too risky to me. But.
> I will be happy if you send this change and take all the blame ;) I won't
> argue (if you limit it to execve case).

Unfortunately you have found what already looks like a userspace
regression.  So I don't think we can do that.

I do think the user space visible change of modifying a multi-threaded
exec no to wait  for the other threads to be reaped is the least
dangerous change.

The big lesson for me, and what was not obvious from your change
description is that we are changing the user space visible semantics
of exec+ptrace and that cred_guard_mutex is not at all the problem (as
we always take cred_guard_mutex in a killable or interruptible way).

So I tentatively agree that we need to deliberately change the semantics
of exec to not wait for zombies in fs/exec.c:de_thread.  That is what I
would expect of exec and that seems to the minimal change.

As part of that change I expect we want to call __cleanup_sighand from
do_exit rather than release_task.  Semantically we sould not need the
sighand_struct for zombie process.

>> --- a/kernel/exit.c
>> +++ b/kernel/exit.c
>> @@ -690,7 +690,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>>  				thread_group_empty(tsk) &&
>>  				!ptrace_reparented(tsk) ?
>>  			tsk->exit_signal : SIGCHLD;
>> -		autoreap = do_notify_parent(tsk, sig);
>> +		do_notify_parent(tsk, sig);
>> +		/* Autoreap threads even when ptraced */
>> +		autoreap = !thread_group_leader(tsk);
>>  	} else if (thread_group_leader(tsk)) {
>>  		autoreap = thread_group_empty(tsk) &&
>>  			do_notify_parent(tsk, tsk->exit_signal);
>
> This is all you need,
>
>> @@ -699,8 +701,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>>  	}
>>
>>  	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;
>> -	if (tsk->exit_state == EXIT_DEAD)
>> -		list_add(&tsk->ptrace_entry, &dead);
>>
>>  	/* mt-exec, de_thread() is waiting for group leader */
>>  	if (unlikely(tsk->signal->notify_count < 0))
>> @@ -711,6 +711,8 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>>  		list_del_init(&p->ptrace_entry);
>>  		release_task(p);
>>  	}
>> +	if (autoreap)
>> +		release_task(tsk);
>
> These 2 changes are not needed. release_task(tsk) will be called by
> list_for_each_entry_safe() above if autoreap == T.

Except for the practical case that for threads that are ptraced
tsk->ptrace_entry is already in use.  Which means we can't use
list_add(&tsk->ptrace_entry, &dead).

Or in short we get a really pretty oops because we corrupt one of the
ptrace lists if we don't have my extra two hunks.  But I agree logically
they are separate changes.

Eric

[toc] | [prev] | [next] | [standalone]

#1592271 — [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped.

Ever since CLONE_THREAD support was added to the kernel it has been
possible to dead-lock userspace by ptracing a process and not reaping
it's child threads.  With use of the cred_guard_mutex in proc the ways
userspace can unknowningly trigger a dead-lock have grown.  Which makes
a simple -f strace on a program that performs a mult-threaded exec
unsafe.

        #include <unistd.h>
        #include <signal.h>
        #include <sys/ptrace.h>
        #include <pthread.h>

        void *thread(void *arg)
        {
                ptrace(PTRACE_TRACEME, 0,0,0);
                return NULL;
        }

        int main(void)
        {
                int pid = fork();

                if (!pid) {
                        pthread_t pt;
                        pthread_create(&pt, NULL, thread, NULL);
                        pthread_join(pt, NULL);
                        execlp("echo", "echo", "passed", NULL);
                }

                sleep(1);
                // or anything else which needs ->cred_guard_mutex,
                // say open(/proc/$pid/mem)
                ptrace(PTRACE_ATTACH, pid, 0,0);
                kill(pid, SIGCONT);

                return 0;
        }

Sovle this by modifying exec to only wait until all of the other
threads are zombies, and not waiting until the other threads
are reaped.

As well as simplifying the userspace semantics this simplifies
the code.

Reported-by: Ulrich Obergfell <uobergfe@redhat.com>
Reported-by: Attila Fazekas <afazekas@redhat.com>
Reported-by: Aleksa Sarai <asarai@suse.com>
Reported-by: Oleg Nesterov <oleg@redhat.com>
Fixes: v2.4.0
Cc: stable@vger.kernel.org
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c             | 31 +++++--------------------------
 include/linux/sched.h |  5 ++---
 kernel/exit.c         | 18 +++++-------------
 kernel/signal.c       |  6 +-----
 4 files changed, 13 insertions(+), 47 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 698a86094f76..f78205a72304 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1065,11 +1065,8 @@ static int de_thread(struct task_struct *tsk)
 	}
 
 	sig->group_exit_task = tsk;
-	sig->notify_count = zap_other_threads(tsk);
-	if (!thread_group_leader(tsk))
-		sig->notify_count--;
-
-	while (sig->notify_count) {
+	zap_other_threads(tsk);
+	while (atomic_read(&sig->live) > 1) {
 		__set_current_state(TASK_KILLABLE);
 		spin_unlock_irq(lock);
 		schedule();
@@ -1081,29 +1078,13 @@ static int de_thread(struct task_struct *tsk)
 
 	/*
 	 * At this point all other threads have exited, all we have to
-	 * do is to wait for the thread group leader to become inactive,
-	 * and to assume its PID:
+	 * do is to assume the PID of the thread group leader.
 	 */
 	if (!thread_group_leader(tsk)) {
 		struct task_struct *leader = tsk->group_leader;
 
-		for (;;) {
-			threadgroup_change_begin(tsk);
-			write_lock_irq(&tasklist_lock);
-			/*
-			 * Do this under tasklist_lock to ensure that
-			 * exit_notify() can't miss ->group_exit_task
-			 */
-			sig->notify_count = -1;
-			if (likely(leader->exit_state))
-				break;
-			__set_current_state(TASK_KILLABLE);
-			write_unlock_irq(&tasklist_lock);
-			threadgroup_change_end(tsk);
-			schedule();
-			if (unlikely(__fatal_signal_pending(tsk)))
-				goto killed;
-		}
+		threadgroup_change_begin(tsk);
+		write_lock_irq(&tasklist_lock);
 
 		/*
 		 * The only record we have of the real-time age of a
@@ -1163,7 +1144,6 @@ static int de_thread(struct task_struct *tsk)
 	}
 
 	sig->group_exit_task = NULL;
-	sig->notify_count = 0;
 
 no_thread_group:
 	/* we have changed execution domain */
@@ -1204,7 +1184,6 @@ static int de_thread(struct task_struct *tsk)
 	/* protects against exit_notify() and __exit_signal() */
 	read_lock(&tasklist_lock);
 	sig->group_exit_task = NULL;
-	sig->notify_count = 0;
 	read_unlock(&tasklist_lock);
 	return -EAGAIN;
 }
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6261bfc12853..ff6aa76beac5 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -711,11 +711,10 @@ struct signal_struct {
 	/* thread group exit support */
 	int			group_exit_code;
 	/* overloaded:
-	 * - notify group_exit_task when ->count is equal to notify_count
+	 * - notify group_exit_task when ->live is equal to 1
 	 * - everyone except group_exit_task is stopped during signal delivery
 	 *   of fatal signals, group_exit_task processes the signal.
 	 */
-	int			notify_count;
 	struct task_struct	*group_exit_task;
 
 	/* thread group stop support, overloads group_exit_code too */
@@ -2763,7 +2762,7 @@ extern __must_check bool do_notify_parent(struct task_struct *, int);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent);
 extern void force_sig(int, struct task_struct *);
 extern int send_sig(int, struct task_struct *, int);
-extern int zap_other_threads(struct task_struct *p);
+extern void zap_other_threads(struct task_struct *p);
 extern struct sigqueue *sigqueue_alloc(void);
 extern void sigqueue_free(struct sigqueue *);
 extern int send_sigqueue(struct sigqueue *,  struct task_struct *, int group);
diff --git a/kernel/exit.c b/kernel/exit.c
index 5cfbd595f918..dc9bc2bdb45a 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -111,13 +111,6 @@ static void __exit_signal(struct task_struct *tsk)
 		tty = sig->tty;
 		sig->tty = NULL;
 	} else {
-		/*
-		 * If there is any task waiting for the group exit
-		 * then notify it:
-		 */
-		if (sig->notify_count > 0 && !--sig->notify_count)
-			wake_up_process(sig->group_exit_task);
-
 		if (tsk == sig->curr_target)
 			sig->curr_target = next_thread(tsk);
 	}
@@ -701,10 +694,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;
 	if (tsk->exit_state == EXIT_DEAD)
 		list_add(&tsk->ptrace_entry, &dead);
-
-	/* mt-exec, de_thread() is waiting for group leader */
-	if (unlikely(tsk->signal->notify_count < 0))
-		wake_up_process(tsk->signal->group_exit_task);
 	write_unlock_irq(&tasklist_lock);
 
 	list_for_each_entry_safe(p, n, &dead, ptrace_entry) {
@@ -740,7 +729,7 @@ static inline void check_stack_usage(void) {}
 void __noreturn do_exit(long code)
 {
 	struct task_struct *tsk = current;
-	int group_dead;
+	int group_left, group_dead;
 	TASKS_RCU(int tasks_rcu_i);
 
 	profile_task_exit(tsk);
@@ -809,7 +798,8 @@ void __noreturn do_exit(long code)
 	if (tsk->mm)
 		sync_mm_rss(tsk->mm);
 	acct_update_integrals(tsk);
-	group_dead = atomic_dec_and_test(&tsk->signal->live);
+	group_left = atomic_dec_return(&tsk->signal->live);
+	group_dead = group_left == 0;
 	if (group_dead) {
 #ifdef CONFIG_POSIX_TIMERS
 		hrtimer_cancel(&tsk->signal->real_timer);
@@ -818,6 +808,8 @@ void __noreturn do_exit(long code)
 		if (tsk->mm)
 			setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm);
 	}
+	if ((group_left == 1) && tsk->signal->group_exit_task)
+		wake_up_process(tsk->signal->group_exit_task);
 	acct_collect(code, group_dead);
 	if (group_dead)
 		tty_audit_exit();
diff --git a/kernel/signal.c b/kernel/signal.c
index fbbab5a7c84d..17312c71f1ae 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1191,16 +1191,14 @@ force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
 /*
  * Nuke all other threads in the group.
  */
-int zap_other_threads(struct task_struct *p)
+void zap_other_threads(struct task_struct *p)
 {
 	struct task_struct *t = p;
-	int count = 0;
 
 	p->signal->group_stop_count = 0;
 
 	while_each_thread(p, t) {
 		task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
-		count++;
 
 		/* Don't bother with already dead threads */
 		if (t->exit_state)
@@ -1208,8 +1206,6 @@ int zap_other_threads(struct task_struct *p)
 		sigaddset(&t->pending.signal, SIGKILL);
 		signal_wake_up(t, 1);
 	}
-
-	return count;
 }
 
 struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1592587 — Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped.

On 03/03, Eric W. Biederman wrote:
>
> Ever since CLONE_THREAD support was added to the kernel it has been
> possible to dead-lock userspace by ptracing a process and not reaping
> it's child threads.

Hmm. I disagree... I do not think this is a bug. But lets discuss this
separately, perhaps I misunderstood you.

> With use of the cred_guard_mutex in proc the ways
> userspace can unknowningly trigger a dead-lock have grown.

I think this particular problem did not exist until cred_guard_mutex
was introduced. Debugger can obviously "delay" exec if it doesn't
reap a zombie sub-thread, but this is another thing and not a bug imo.


> Sovle this by modifying exec to only wait until all of the other
> threads are zombies, and not waiting until the other threads
> are reaped.

This patch looks wrong in many ways.

> @@ -1065,11 +1065,8 @@ static int de_thread(struct task_struct *tsk)
>  	}
>
>  	sig->group_exit_task = tsk;
> -	sig->notify_count = zap_other_threads(tsk);
> -	if (!thread_group_leader(tsk))
> -		sig->notify_count--;
> -
> -	while (sig->notify_count) {
> +	zap_other_threads(tsk);
> +	while (atomic_read(&sig->live) > 1) {
>  		__set_current_state(TASK_KILLABLE);
>  		spin_unlock_irq(lock);
>  		schedule();

Very nice. So de_thread() returns as soon as all other threads decrement
signal->live in do_exit(). Before they do, say, exit_mm(). This is already
wrong, for example this breaks OOM. Plus a lot more problems afaics,  but
lets ignore this.

Note that de_thread() also unshares ->sighand before return. So in the
case of mt exec it will likely see oldsighand->count != 1 and alloc the
new sighand_struct and this breaks the locking.

Because the execing thread will use newsighand->siglock to protect its
signal_struct while the zombie threads will use oldsighand->siglock to
protect the same signal struct. Yes, tasklist_lock + the fact irq_disable
implies rcu_lock mostly save us but not entirely, say, a foreign process
doing __send_signal() can take the right or the wrong lock depending on
/dev/random.


> @@ -818,6 +808,8 @@ void __noreturn do_exit(long code)
>  		if (tsk->mm)
>  			setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm);
>  	}
> +	if ((group_left == 1) && tsk->signal->group_exit_task)
> +		wake_up_process(tsk->signal->group_exit_task);

This is racy, but this is minor.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1592287

ebiederm@xmission.com (Eric W. Biederman) writes:

> ebiederm@xmission.com (Eric W. Biederman) writes:
>
>> The big lesson for me, and what was not obvious from your change
>> description is that we are changing the user space visible semantics
>> of exec+ptrace and that cred_guard_mutex is not at all the problem (as
>> we always take cred_guard_mutex in a killable or interruptible way).
>
> Just to follow up.
>
> Because the cred_guard_mutex is fine as is we don't need to move
> de_thread out from under cred_guard_mutex.  We just need to change
> de_thread to wait until all of the other threads are zombies.
> Which should remove about half your proposed patch.
>
> The other key thing is that knowning it isn't cred_guard_mutex let's us
> know that this kind of deadlock goes all of the way back to when
> CLONE_THREAD was merged into the kernel.
>
> Insteresingly enough looking at zap_other_threads and notify_count I
> have found a second bug.  When a multi-threaded processes becomes a
> zombie we don't send the notification to the parent process until the
> non-leader threads have been reaped.  Which means ptrace can mess up
> sending SIGCHLD to the parent.

Bah. I was misreading the code.  Nothing but exec uses notify_count
and group_exit_task.

Eric

[toc] | [prev] | [next] | [standalone]

#1592347

ebiederm@xmission.com (Eric W. Biederman) writes:

> The big lesson for me, and what was not obvious from your change
> description is that we are changing the user space visible semantics
> of exec+ptrace and that cred_guard_mutex is not at all the problem (as
> we always take cred_guard_mutex in a killable or interruptible way).

Just to follow up.

Because the cred_guard_mutex is fine as is we don't need to move
de_thread out from under cred_guard_mutex.  We just need to change
de_thread to wait until all of the other threads are zombies.
Which should remove about half your proposed patch.

The other key thing is that knowning it isn't cred_guard_mutex let's us
know that this kind of deadlock goes all of the way back to when
CLONE_THREAD was merged into the kernel.

Insteresingly enough looking at zap_other_threads and notify_count I
have found a second bug.  When a multi-threaded processes becomes a
zombie we don't send the notification to the parent process until the
non-leader threads have been reaped.  Which means ptrace can mess up
sending SIGCHLD to the parent.

Now arguably that might be what is desirable but I don't think so.  If
we aren't ptracing a thread then I don't think we want to delay sending
SIGCHLD to the parent.

So this whole area of the semantics of a ptrace'd multi-threaded process
exiting/exec'ing looks like it needs a thorough going over.

Eric

[toc] | [prev] | [next] | [standalone]

#1592584

On 03/03, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> >> @@ -699,8 +701,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
> >>  	}
> >>
> >>  	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;
> >> -	if (tsk->exit_state == EXIT_DEAD)
> >> -		list_add(&tsk->ptrace_entry, &dead);
> >>
> >>  	/* mt-exec, de_thread() is waiting for group leader */
> >>  	if (unlikely(tsk->signal->notify_count < 0))
> >> @@ -711,6 +711,8 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
> >>  		list_del_init(&p->ptrace_entry);
> >>  		release_task(p);
> >>  	}
> >> +	if (autoreap)
> >> +		release_task(tsk);
> >
> > These 2 changes are not needed. release_task(tsk) will be called by
> > list_for_each_entry_safe() above if autoreap == T.
>
> Except for the practical case that for threads that are ptraced
> tsk->ptrace_entry is already in use.  Which means we can't use
> list_add(&tsk->ptrace_entry, &dead).

Yes, I was wrong here, thanks for correcting me.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web