Additional/catchup RCU signal fixes for -mm

kernel/signal.c +1 -1

@@ -1386,7 +1387,7 @@ send_sigqueue(int sig, struct sigqueue *
 {
 	unsigned long flags;
 	int ret = 0;
-	struct sighand_struct *sh = p->sighand;
+	struct sighand_struct *sh;
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 

@@ -1405,7 +1406,15 @@ send_sigqueue(int sig, struct sigqueue *
 		goto out_err;

kernel/signal.c +0 -2

@@ -1464,15 +1473,8 @@ send_group_sigqueue(int sig, struct sigq
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 
-	while (!read_trylock(&tasklist_lock)) {
-		if (!p->sighand)

Quoted 14 lines · Paul E. McKenney

(quoted diff) +1 -1

@@ -1386,7 +1387,7 @@ send_sigqueue(int sig, struct sigqueue *
 {
 	unsigned long flags;
 	int ret = 0;
-	struct sighand_struct *sh = p->sighand;
+	struct sighand_struct *sh;
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 

@@ -1405,7 +1406,15 @@ send_sigqueue(int sig, struct sigqueue *
 		goto out_err;
 	}
 

Review send_sigqueue(int sig, struct sigqueue * · line 1416

Quoted 8 lines

(quoted diff) +7 -0

+retry:
+	sh = rcu_dereference(p->sighand);
+
 	spin_lock_irqsave(&sh->siglock, flags);
+	if (p->sighand != sh) {
+		/* We raced with exec() in a multithreaded process... */
+		spin_unlock_irqrestore(&sh->siglock, flags);
+		goto retry;

p->sighand can't be changed, de_thread calls exit_itimers() before changing ->sighand. But I still think it can be NULL, and send_sigqueue() should return -1 in that case.

Quoted 6 lines

(quoted diff) +0 -2

@@ -1464,15 +1473,8 @@ send_group_sigqueue(int sig, struct sigq
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 
-	while (!read_trylock(&tasklist_lock)) {
-		if (!p->sighand)

Review send_group_sigqueue(int sig, struct sigq · line 1478

Quoted 10 lines

(quoted diff) +2 -7

-			return -1;
-		cpu_relax();
-	}
-	if (unlikely(!p->sighand)) {
-		ret = -1;
-		goto out_err;
-	}
+	read_lock(&tasklist_lock);
+	/* Since it_lock is held, p->sighand cannot be NULL. */
 	spin_lock_irqsave(&p->sighand->siglock, flags);

Again, I think the comment is wrong.

However, now I think we really have a race with exec, and this race was not introduced by your patches!

If !thread_group_leader() does exec de_thread() calls release_task(->group_leader) before calling exit_itimers(). This means that send_group_sigqueue() which always has p == ->group_leader parameter can oops here.

Quoted 4 lines

If !thread_group_leader() does exec de_thread() calls release_task(->group_leader) before calling exit_itimers(). This means that send_group_sigqueue() which always has p == ->group_leader parameter can oops here.

But in that case, __exit_sighand(->group_leader) would have been called, so ->sighand would be NULL.

And none of this can change while we are holding
tasklist_lock.

If we don't want to be hitting the exec()ed task with a signal, the thing to do would be to drop the signal, as in the attached patch. I believe that this is an acceptable approach, since had the timer fired slightly later, it would have been disabled, right?

Thoughts?

Thanx, Paul

Signed-off-by <[email protected]>

diff -urpNa -X dontdiff linux-2.6.14-mm0-fix-2/kernel/signal.c linux-2.6.14-mm0-fix-3/kernel/signal.c

kernel/signal.c send_group_sigqueue(int sig, struct sigq · line 1485

Quoted 3 lines · Paul E. McKenney

kernel/signal.c +2 -0

+       if (p->sighand == NULL) {
+               ret = 1;

Oh, I think there is another problem here. I'll post a separate message.

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Paul E. McKenney

kernel/signal.c +2 -0

+       if (p->sighand == NULL) {
+               ret = 1;

Quoted 2 lines

Oh, I think there is another problem here. I'll post a separate message.

Sorry, I was not clear. This problem is unrelated. Yes, I think we should drop the signal. But please note that ret = 1 (sig_ignored) means (surprise!) "reschedule and re-arm this timer right now" in cpu-timer case. It is not critical, but may be ret = 0 is better.

When non-leader thread does exec, de_thread calls release_task(leader) before calling exit_itimers(). If local timer interrupt happens in between, it can oops in send_group_sigqueue() while taking ->sighand->siglock == NULL.

However, we can't change send_group_sigqueue() to check p->signal != NULL, because sys_timer_create() does get_task_struct() only in SIGEV_THREAD_ID case. So it is possible that this task_struct was already freed and we can't trust p->signal.

This patch changes de_thread() so that leader released after exit_itimers() call.

Signed-off-by Oleg Nesterov <[email protected]>

fs/exec.c renamed from fs/exec.c~ +7 -3

@@ -593,6 +593,7 @@ static inline int de_thread(struct task_
 	struct signal_struct *sig = tsk->signal;
 	struct sighand_struct *newsighand, *oldsighand = tsk->sighand;
 	spinlock_t *lock = &oldsighand->siglock;
+	struct task_struct *leader = NULL;
 	int count;
 
 	/*

@@ -668,7 +669,7 @@ static inline int de_thread(struct task_
 	 * and to assume its PID:
 	 */
 	if (!thread_group_leader(current)) {
-		struct task_struct *leader = current->group_leader, *parent;
+		struct task_struct *parent;
 		struct dentry *proc_dentry1, *proc_dentry2;
 		unsigned long exit_state, ptrace;
 

@@ -677,6 +678,7 @@ static inline int de_thread(struct task_
 		 * It should already be zombie at this point, most
 		 * of the time.
 		 */
+		leader = current->group_leader;
 		while (leader->exit_state != EXIT_ZOMBIE)
 			yield();
 

@@ -736,7 +738,6 @@ static inline int de_thread(struct task_
 		proc_pid_flush(proc_dentry2);
 
 		BUG_ON(exit_state != EXIT_ZOMBIE);
-		release_task(leader);
         }
 
 	/*

@@ -746,8 +747,11 @@ static inline int de_thread(struct task_
 	sig->flags = 0;
 
 no_thread_group:
-	BUG_ON(atomic_read(&sig->count) != 1);
 	exit_itimers(sig);
+	if (leader)
+		release_task(leader);
+
+	BUG_ON(atomic_read(&sig->count) != 1);
 
 	if (atomic_read(&oldsighand->count) == 1) {
 		/*

Quoted 9 lines · * Oleg Nesterov ([email protected]) wrote:

When non-leader thread does exec, de_thread calls release_task(leader) before calling exit_itimers(). If local timer interrupt happens in between, it can oops in send_group_sigqueue() while taking ->sighand->siglock == NULL.

However, we can't change send_group_sigqueue() to check p->signal != NULL, because sys_timer_create() does get_task_struct() only in SIGEV_THREAD_ID case. So it is possible that this task_struct was already freed and we can't trust p->signal.

Quoted 2 lines

This patch changes de_thread() so that leader released after exit_itimers() call.

Nice catch. As soon as Linus picks it up we'll put it in -stable as well.

While rooting aroung in the signal code trying to understand how to
fix the SIG_IGN ploy (set sig handler to SIG_IGN and flood system with
high speed repeating timers) I came across what, I think, is a problem in sigaction() in that when processing a SIG_IGN request it flushes signals from 1 to SIGRTMIN and leaves the rest.

Still rooting around in the above. The test program is attached. It creates and arms a repeating timer and then clones a thread which does an exec() call.

first instance of this, we see that the thread-group leader is dead and the exec code at line ~718 is setting the old leaders group-leader to him self.

Quoted 3 lines · George Anzinger

Still rooting around in the above. The test program is attached. It creates and arms a repeating timer and then clones a thread which does an exec() call.

This patch:

	http://marc.theaimsgroup.com/?l=linux-kernel&m=113138286512847

was intended to fix exactly this problem (and the same test program was used to exploit the race and test the fix).

So, it does not help? I can't reproduce the problem.

Note: I think you also need this patch:

	http://marc.theaimsgroup.com/?l=linux-kernel&m=113059955626598

otherwise I beleive OOPS can happen while killing this program if you are running the kernel with this change applied:

	[PATCH] Call exit_itimers from do_exit, not __exit_signal
	http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=25f407f0b668f5e4ebd5d13e1fb4306ba6427ead

Quoted 3 lines

first instance of this, we see that the thread-group leader is dead and the exec code at line ~718 is setting the old leaders group-leader to him self.

I think this code at line ~718

	leader->group_leader = leader;

is noop, because leader->group_leader == leader here.

fs/exec.c static inline int de_thread(struct task_ · line 718

Quoted 2 lines

fs/exec.c +1 -1

-               leader->group_leader = leader;
+               leader->group_leader = current;

This can't help, without SIGEV_THREAD_ID we don't check ->group_leader, the signal goes to the thread group via timer->it_process, which is equal to the old leader.

Quoted 8 lines

This patch:

	http://marc.theaimsgroup.com/?l=linux-kernel&m=113138286512847

was intended to fix exactly this problem (and the same test program was used to exploit the race and test the fix).

So, it does not help? I can't reproduce the problem.

Yes, it does fix it. Somehow I missed the posting of that patch.