[PATCH] Fix setpgid and threads

From: Jeremy Fitzhardinge <jeremy@goop.org>

I'm resending my patch to fix this problem.  To recap: every task_struct
has its own copy of the thread group's pgrp.  Only the thread group
leader is allowed to change the tgrp's pgrp, but it only updates its own
copy of pgrp, while all the other threads in the tgrp use the old value
they inherited on creation.

This patch simply updates all the other thread's pgrp when the tgrp
leader changes pgrp.  Ulrich has already expressed reservations about
this patch since it is (1) incomplete (it doesn't cover the case of
other ids which have similar problems), (2) racy (it doesn't synchronize
with other threads looking at the task pgrp, so they could see an
inconsistent view) and (3) slow (it takes linear time with respect to
the number of threads in the tgrp).

My reaction is that (1) it fixes the actual bug I'm encountering in a
real program.  (2) doesn't really matter for pgrp, since it is mostly an
issue with respect to the terminal job-control code (which is even more
broken without this patch.  Regarding (3), I think there are very few
programs which have a large number of threads which change process group
id on a regular basis (a heavily multi-threaded job-control shell?).

Ulrich also said he has a (proposed?) much better fix, which I've been
looking forward to.  I'm submitting this patch as a stop-gap fix for a
real bug, and perhaps to prompt the improved patch.

An alternative fix, at least for pgrp, is to change all references to
->pgrp to group_leader->pgrp.  This may be sufficient on its own, but it
would be a reasonably intrusive patch (I count 95 instances in 32 files
in the 2.6.0-test3-mm3 tree).

1 files changed, 12 insertions, 12 deletions

diff --git a/kernel/exit.c b/kernel/exit.c
index b6174f82adf9..c565fd69d559 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -152,7 +152,7 @@ static int will_become_orphaned_pgrp(int pgrp, task_t *ignored_task)
 				|| p->state >= TASK_ZOMBIE 
 				|| p->real_parent->pid == 1)
 			continue;
-		if (p->real_parent->pgrp != pgrp
+		if (process_group(p->real_parent) != pgrp
 			    && p->real_parent->session == p->session) {
 			ret = 0;
 			break;
@@ -247,9 +247,9 @@ void __set_special_pids(pid_t session, pid_t pgrp)
 		curr->session = session;
 		attach_pid(curr, PIDTYPE_SID, session);
 	}
-	if (curr->pgrp != pgrp) {
+	if (process_group(curr) != pgrp) {
 		detach_pid(curr, PIDTYPE_PGID);
-		curr->pgrp = pgrp;
+		curr->group_leader->__pgrp = pgrp;
 		attach_pid(curr, PIDTYPE_PGID, pgrp);
 	}
 }
@@ -508,9 +508,9 @@ static inline void reparent_thread(task_t *p, task_t *father, int traced)
 	 * than we are, and it was the only connection
 	 * outside, so the child pgrp is now orphaned.
 	 */
-	if ((p->pgrp != father->pgrp) &&
+	if ((process_group(p) != process_group(father)) &&
 	    (p->session == father->session)) {
-		int pgrp = p->pgrp;
+		int pgrp = process_group(p);
 
 		if (will_become_orphaned_pgrp(pgrp, NULL) && has_stopped_jobs(pgrp)) {
 			__kill_pg_info(SIGHUP, (void *)1, pgrp);
@@ -618,12 +618,12 @@ static void exit_notify(struct task_struct *tsk)
 	 
 	t = tsk->real_parent;
 	
-	if ((t->pgrp != tsk->pgrp) &&
+	if ((process_group(t) != process_group(tsk)) &&
 	    (t->session == tsk->session) &&
-	    will_become_orphaned_pgrp(tsk->pgrp, tsk) &&
-	    has_stopped_jobs(tsk->pgrp)) {
-		__kill_pg_info(SIGHUP, (void *)1, tsk->pgrp);
-		__kill_pg_info(SIGCONT, (void *)1, tsk->pgrp);
+	    will_become_orphaned_pgrp(process_group(tsk), tsk) &&
+	    has_stopped_jobs(process_group(tsk))) {
+		__kill_pg_info(SIGHUP, (void *)1, process_group(tsk));
+		__kill_pg_info(SIGCONT, (void *)1, process_group(tsk));
 	}
 
 	/* Let father know we died 
@@ -813,10 +813,10 @@ static int eligible_child(pid_t pid, int options, task_t *p)
 		if (p->pid != pid)
 			return 0;
 	} else if (!pid) {
-		if (p->pgrp != current->pgrp)
+		if (process_group(p) != process_group(current))
 			return 0;
 	} else if (pid != -1) {
-		if (p->pgrp != -pid)
+		if (process_group(p) != -pid)
 			return 0;
 	}