ptrace: don't clear GROUP_STOP_SIGMASK on double-stop

PATCH In review Oleg Nesterov 3 participants 6 messages Sep 26, 2011 – Oct 20, 2011

lore

Share & tools

lore

Oleg Nesterov

author

2011-09-26 17:34 UTC #1 lore raw

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

1. Luke Macken triggered WARN_ON(!(group_stop & GROUP_STOP_SIGMASK)) in do_signal_stop().
This is because do_signal_stop() clears GROUP_STOP_SIGMASK part unconditionally but doesn't update it if task_is_stopped().

Quoted 6 lines

2. Looking at this problem I noticed that WARN_ON_ONCE(!ptrace) is not right, a stopped-but-resumed tracee can clone the untraced thread in the SIGNAL_STOP_STOPPED group, the new thread can start another group-stop.
Remove this warning, we need more fixes to make it true.

Tejun, 3.1 has similar problems. I'll write another email...

Oleg.

Oleg Nesterov

author

2011-09-26 17:35 UTC #0 lore raw

PATCH 1/1 ptrace: don't clear GROUP_STOP_SIGMASK on double-stop

https://bugzilla.redhat.com/show_bug.cgi?id=740121

1. Luke Macken triggered WARN_ON(!(group_stop & GROUP_STOP_SIGMASK)) in do_signal_stop().

This is because do_signal_stop() clears GROUP_STOP_SIGMASK part unconditionally but doesn't update it if task_is_stopped().

2. Looking at this problem I noticed that WARN_ON_ONCE(!ptrace) is not right, a stopped-but-resumed tracee can clone the untraced thread in the SIGNAL_STOP_STOPPED group, the new thread can start another group-stop.

Remove this warning, we need more fixes to make it true.

Reported-by Luke Macken <[email protected]>

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

kernel/signal.c +1 -3

@@ -1894,21 +1894,19 @@ static int do_signal_stop(int signr)
 		 */
 		if (!(sig->flags & SIGNAL_STOP_STOPPED))
 			sig->group_exit_code = signr;
-		else
-			WARN_ON_ONCE(!task_ptrace(current));
 
 		current->group_stop &= ~GROUP_STOP_SIGMASK;
 		current->group_stop |= signr | gstop;
 		sig->group_stop_count = 1;
 		for (t = next_thread(current); t != current;
 		     t = next_thread(t)) {
-			t->group_stop &= ~GROUP_STOP_SIGMASK;
 			/*
 			 * Setting state to TASK_STOPPED for a group
 			 * stop is always done with the siglock held,
 			 * so this check has no races.
 			 */
 			if (!(t->flags & PF_EXITING) && !task_is_stopped(t)) {
+				t->group_stop &= ~GROUP_STOP_SIGMASK;
 				t->group_stop |= signr | gstop;
 				sig->group_stop_count++;
 				signal_wake_up(t, 0);

Greg KH

2011-09-26 22:15 UTC #2 lore raw

Quoted 13 lines · Oleg Nesterov

https://bugzilla.redhat.com/show_bug.cgi?id=740121
1. Luke Macken triggered WARN_ON(!(group_stop & GROUP_STOP_SIGMASK)) in do_signal_stop().
This is because do_signal_stop() clears GROUP_STOP_SIGMASK part unconditionally but doesn't update it if task_is_stopped().
2. Looking at this problem I noticed that WARN_ON_ONCE(!ptrace) is not right, a stopped-but-resumed tracee can clone the untraced thread in the SIGNAL_STOP_STOPPED group, the new thread can start another group-stop.

Quoted 4 lines

Remove this warning, we need more fixes to make it true.
Reported-by Luke Macken <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>

Is this fix in Linus's tree already? If so, what's the git commit id?

If not, please take a look at Documentation/stable_kernel_rules.txt for how to properly submit a patch to the stable tree.

thanks,

greg k-h

Oleg Nesterov

author

2011-09-27 12:58 UTC #3 lore raw

Quoted 15 lines · Greg KH

Quoted 13 lines · Oleg Nesterov
https://bugzilla.redhat.com/show_bug.cgi?id=740121
1. Luke Macken triggered WARN_ON(!(group_stop & GROUP_STOP_SIGMASK)) in do_signal_stop().
This is because do_signal_stop() clears GROUP_STOP_SIGMASK part unconditionally but doesn't update it if task_is_stopped().
2. Looking at this problem I noticed that WARN_ON_ONCE(!ptrace) is not right, a stopped-but-resumed tracee can clone the untraced thread in the SIGNAL_STOP_STOPPED group, the new thread can start another group-stop.

Quoted 6 lines

Quoted 4 lines
Remove this warning, we need more fixes to make it true.
Reported-by Luke Macken <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Is this fix in Linus's tree already? If so, what's the git commit id?

No. As I said, 3.1 has the similar problem. But not the same, and you need the quite different test-case to trigger the bug.

We are discussing the possible fixes for 3.1, but this code was changed very much and it doesn't make sense to compare the fixes for 3.1 and 3.0.

Oleg.

Greg KH

2011-10-19 23:03 UTC #4 lore raw

In reply to Oleg Nesterov

Quoted 13 lines · Oleg Nesterov

https://bugzilla.redhat.com/show_bug.cgi?id=740121
1. Luke Macken triggered WARN_ON(!(group_stop & GROUP_STOP_SIGMASK)) in do_signal_stop().
This is because do_signal_stop() clears GROUP_STOP_SIGMASK part unconditionally but doesn't update it if task_is_stopped().
2. Looking at this problem I noticed that WARN_ON_ONCE(!ptrace) is not right, a stopped-but-resumed tracee can clone the untraced thread in the SIGNAL_STOP_STOPPED group, the new thread can start another group-stop.

Quoted 4 lines

Remove this warning, we need more fixes to make it true.
Reported-by Luke Macken <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>

I still don't understand, did this fix ever get into Linus's tree? If so, what is the git commit id.

If not, why not? I can't apply it to the 3.0-stable tree unless it is there, or there is a very good reason why it isn't.

thanks,

greg k-h

Oleg Nesterov

author

2011-10-20 14:56 UTC #5 lore raw

Quoted 17 lines · Greg KH

Quoted 15 lines · Oleg Nesterov
https://bugzilla.redhat.com/show_bug.cgi?id=740121
1. Luke Macken triggered WARN_ON(!(group_stop & GROUP_STOP_SIGMASK)) in do_signal_stop().
This is because do_signal_stop() clears GROUP_STOP_SIGMASK part unconditionally but doesn't update it if task_is_stopped().
2. Looking at this problem I noticed that WARN_ON_ONCE(!ptrace) is not right, a stopped-but-resumed tracee can clone the untraced thread in the SIGNAL_STOP_STOPPED group, the new thread can start another group-stop.
Remove this warning, we need more fixes to make it true.

Quoted 7 lines

Quoted 2 lines
Reported-by Luke Macken <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
I still don't understand, did this fix ever get into Linus's tree? If so, what is the git commit id.
If not, why not?

I already explained:

No. As I said, 3.1 has the similar problem. But not the same, and you need the quite different test-case to trigger the bug.

We are discussing the possible fixes for 3.1, but this code was changed very much and it doesn't make sense to compare the fixes for 3.1 and 3.0.

IOW. This code was rewritten in 3.1, this particlular problem was "fixed" by accident, but in fact the problem just mutated.

Quoted 2 lines

I can't apply it to the 3.0-stable tree unless it is there, or there is a very good reason why it isn't.

OK, please ignore then.

Oleg.