public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Linus Torvalds <torvalds@osdl.org>
Cc: Oleg Nesterov <oleg@tv-sign.ru>, Andrew Morton <akpm@osdl.org>,
	linux-kernel@vger.kernel.org
Subject: [PATCH] de_thread: Don't confuse users do_each_thread.
Date: Mon, 10 Apr 2006 17:16:49 -0600	[thread overview]
Message-ID: <m1u091dnry.fsf@ebiederm.dsl.xmission.com> (raw)
In-Reply-To: <m1y7yddo75.fsf_-_@ebiederm.dsl.xmission.com> (Eric W. Biederman's message of "Mon, 10 Apr 2006 17:07:42 -0600")


I can't seem to send out a correct patch out today with out
sending it twice.  I accidently grabbed my old version
that sent to many arguments to detach_pid and so would not
compile.  Oops.

---

Oleg Nesterov spotted two interesting bugs with the current de_thread
code.  The simplest is a long standing double decrement of
__get_cpu_var(process_counts) in __unhash_process.  Caused by
two processes exiting when only one was created.

The other is that since we no longer detach from the thread_group list
it is possible for do_each_thread when run under the tasklist_lock to
see the same task_struct twice.  Once on the task list as a
thread_group_leader, and once on the thread list of another
thread.

The double appearance in do_each_thread can cause a double increment
of mm_core_waiters in zap_threads resulting in problems later on in
coredump_wait.

To remedy those two problems this patch takes the simple approach
of changing the old thread group leader into a child thread.
The only routine in release_task that cares is __unhash_process,
and it can be trivially seen that we handle cleaning up a
thread group leader properly.

Since de_thread doesn't change the pid of the exiting leader process
and instead shares it with the new leader process.  I change
thread_group_leader to recognize group leadership based on the
group_leader field and not based on pids.  This should also be
slightly cheaper then the existing thread_group_leader macro.

I performed a quick audit and I couldn't see any user of
thread_group_leader that cared how cared about the difference.

I believe this is 2.6.17 material as the bug is present in
2.6.17-rc1 and the fix is simple.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/exec.c             |    7 ++++++-
 include/linux/sched.h |    3 ++-
 2 files changed, 8 insertions(+), 2 deletions(-)

e621f800b1de6684beb995014190b3ccaad5c0b3
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 541f482..a3e4f6b 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1203,7 +1203,8 @@ #define do_each_thread(g, t) \
 #define while_each_thread(g, t) \
 	while ((t = next_thread(t)) != g)
 
-#define thread_group_leader(p)	(p->pid == p->tgid)
+/* de_thread depends on thread_group_leader not being a pid based check */
+#define thread_group_leader(p)	(p == p->group_leader)
 
 static inline task_t *next_thread(task_t *p)
 {
diff --git a/fs/exec.c b/fs/exec.c
index 0291a68..4d38ad0 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -723,7 +723,12 @@ static int de_thread(struct task_struct 
 		current->parent = current->real_parent = leader->real_parent;
 		leader->parent = leader->real_parent = child_reaper;
 		current->group_leader = current;
-		leader->group_leader = leader;
+		leader->group_leader = current;
+
+		/* Reduce leader to a thread */
+		detach_pid(leader, PIDTYPE_PGID);
+		detach_pid(leader, PIDTYPE_SID);
+		list_del_init(&leader->tasks);
 
 		add_parent(current);
 		add_parent(leader);
-- 
1.3.0.rc3.g6b3a


  reply	other threads:[~2006-04-10 23:18 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-04-06 22:04 [PATCH rc1-mm] de_thread: fix deadlockable process addition Oleg Nesterov
2006-04-06 22:58 ` Eric W. Biederman
2006-04-07 23:46   ` Oleg Nesterov
2006-04-07 22:51     ` Andrew Morton
2006-04-07 22:56       ` Andrew Morton
2006-04-08  7:55         ` Eric W. Biederman
2006-04-08 17:27           ` Oleg Nesterov
2006-04-08 16:07             ` Eric W. Biederman
2006-04-08 21:13               ` Oleg Nesterov
2006-04-08 21:23                 ` Oleg Nesterov
2006-04-10 22:11                   ` Eric W. Biederman
2006-04-10 23:07             ` [PATCH] de_thread: Don't confuse users do_each_thread Eric W. Biederman
2006-04-10 23:16               ` Eric W. Biederman [this message]
2006-04-10 23:52                 ` Ingo Oeser
2006-04-11  6:18                   ` Eric W. Biederman
2006-04-11 10:19                   ` Oleg Nesterov
2006-04-11  7:25                     ` Ingo Oeser
2006-04-11  7:36                       ` Eric W. Biederman
2006-04-11 19:50                         ` Ingo Oeser
2006-04-11 10:05                 ` Oleg Nesterov
2006-04-11  5:23                   ` Andrew Morton
2006-04-11 10:47                     ` Oleg Nesterov
2006-04-11  6:23                   ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m1u091dnry.fsf@ebiederm.dsl.xmission.com \
    --to=ebiederm@xmission.com \
    --cc=akpm@osdl.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@tv-sign.ru \
    --cc=torvalds@osdl.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox