Re: PATCH? tracehook_notify_jctl && SIGCONT

[Roland McGrath]

> 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.

Yes, I considered this problem.  It's just not so big a deal as to worry
overmuch about this corner case in the first version.  What seems to me
will be the obvious and straightforward way to address it is to give
utrace_report_jctl() a return value that tracehook_notify_jctl() uses.
Then we can omit a notification that has been superceded.

Your patch does not seem very straightforward to me.  Moreover, you moved
some ptrace magic out of the tracehook function back into core signals code.
That is going in the wrong direction and we won't have any of that.


Thanks,
Roland

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.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(¤t->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);