PATCH? tracehook_notify_jctl && SIGCONT

[Oleg Nesterov]

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.000000000 
+0100
+++ xxx/include/linux/tracehook.h       2009-03-18 20:18:54.000000000 +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.000000000 +0100
+++ xxx/kernel/signal.c 2009-03-18 20:28:39.000000000 +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);