On 03/18, Oleg Nesterov wrote:
On 03/18, Roland McGrath wrote:
It's OK to change the tracehook definition. I did this on the new git
branch tracehook, then utrace branch commit 7b0be6e4 merges that and uses
it.
Roland, I think it better to change tracehook definition more, please
see below.
The problem is that, since utrace_report_jctl() drops -siglock,
tracehook_notify_jctl() can return false positive. This is easy
to fix, but then we have to check PT_PTRACED twice, not good.
Suppose we have 2 threads, T1 and T2, T1 has JCTL in -utrace_flags.
T2 dequeues SIGSTOP, calls do_signal_stop(), and sleeps in TASK_STOPPED.
T1 calls do_signal_stop(). -group_stop_count == 1, so it does
notify = tracehook_notify_jctl(notify = CLD_STOPPED), this means
that notify always becomes CLD_STOPPED.
When tracehook_notify_jctl()-utrace_notify_jctl() drops siglock,
SIGCONT comes, notices -group_stop_count != 0, and adds SIGNAL_CLD_STOPPED
to signal flags.
Now we send SIGCHLD with si_code = CLD_STOPPED twice. By T1 from
do_signal_stop(), and by T1 or T2 from get_signal_to_deliver() which
checks SIGNAL_CLD_MASK.
I'd suggest something like the patch below. At least for now.
Oleg.
--- xxx/include/linux/tracehook.h~JCTL_NOTIFY 2009-03-18 14:50:05.0
+0100
+++ xxx/include/linux/tracehook.h 2009-03-18 20:18:54.0 +0100
@@ -520,11 +520,10 @@ static inline int tracehook_get_signal(s
*
* Called with the siglock held.
*/
-static inline int tracehook_notify_jctl(int notify, int why)
+static inline void tracehook_notify_jctl(int notify, int why)
{
if (task_utrace_flags(current) UTRACE_EVENT(JCTL))
utrace_report_jctl(notify, why);
- return notify ?: (current-ptrace PT_PTRACED) ? why : 0;
}
#define DEATH_REAP -1
--- xxx/kernel/signal.c~JCTL_NOTIFY 2009-03-18 18:20:35.0 +0100
+++ xxx/kernel/signal.c 2009-03-18 20:28:39.0 +0100
@@ -1671,18 +1671,21 @@ static int do_signal_stop(int signr)
* a group stop in progress and we are the last to stop,
* report to the parent. When ptraced, every thread reports itself.
*/
- notify = sig-group_stop_count == 1 ? CLD_STOPPED : 0;
- notify = tracehook_notify_jctl(notify, CLD_STOPPED);
+ tracehook_notify_jctl(sig-group_stop_count == 1 ? CLD_STOPPED : 0,
+ CLD_STOPPED);
+ notify = 0;
if (sig-group_stop_count) {
- if (!--sig-group_stop_count)
+ if (!--sig-group_stop_count) {
sig-flags = SIGNAL_STOP_STOPPED;
+ notify = 1;
+ }
current-exit_code = sig-group_exit_code;
__set_current_state(TASK_STOPPED);
}
spin_unlock_irq(current-sighand-siglock);
- if (notify) {
+ if (notify || (current-ptrace PT_PTRACED)) {
read_lock(tasklist_lock);
do_notify_parent_cldstop(current, notify);
read_unlock(tasklist_lock);
@@ -1765,14 +1768,12 @@ relock:
? CLD_CONTINUED : CLD_STOPPED;
signal-flags = ~SIGNAL_CLD_MASK;
- why = tracehook_notify_jctl(why, CLD_CONTINUED);
+ tracehook_notify_jctl(why, CLD_CONTINUED);
spin_unlock_irq(sighand-siglock);
- if (why) {
- read_lock(tasklist_lock);
- do_notify_parent_cldstop(current-group_leader, why);
- read_unlock(tasklist_lock);
- }
+ read_lock(tasklist_lock);
+ do_notify_parent_cldstop(current-group_leader, why);
+ read_unlock(tasklist_lock);
goto relock;
}
@@ -1930,7 +1931,8 @@ void exit_signals(struct task_struct *ts
if (unlikely(tsk-signal-group_stop_count)
!--tsk-signal-group_stop_count) {
tsk-signal-flags = SIGNAL_STOP_STOPPED;
- group_stop = tracehook_notify_jctl(CLD_STOPPED, CLD_STOPPED);
+ tracehook_notify_jctl(CLD_STOPPED, CLD_STOPPED);
+ group_stop = 1;
}
out:
spin_unlock_irq(tsk-sighand-siglock);