Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Tue, 21 Dec 2010 18:25:16 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 14/16] ptrace: make SIGCONT notification reliable against ptrace |
| |
On 12/06, Tejun Heo wrote: > > This patch adds a new signal flag SIGNAL_NOTIFY_CONT which is set when > a task is woken up by SIGCONT and cleared once the event is notified > to the parent. SIGNAL_CLD_MASK bits are no longer cleared after > notification. Combined with clearing SIGNAL_CLD_MASK if > !SIGNAL_NOTIFY_CONT on ptrace attach, these bits are set on ptrace > detach iff the tracee owes a notification to the real parent.
But we can't know this. The notification and SIGNAL_NOTIFY_CONT are per-process, while attach/detach is per-thread.
> @@ -66,6 +67,33 @@ void __ptrace_unlink(struct task_struct *child) > if (sig->flags & SIGNAL_STOP_STOPPED || sig->group_stop_count) > child->group_stop |= GROUP_STOP_PENDING; > signal_wake_up(child, 1); > + woken_up = true; > + } > + > + /* > + * SIGNAL_CLD_MASK is cleared only on a stop signal or, if > + * notification isn't pending, ptrace attach. If any bit is > + * set, > + * > + * - SIGCONT notification was pending before attach or there > + * was one or more SIGCONT notifications while tracing. > + * > + * - And, there hasn't been any stop signal since the last > + * pending SIGCONT notification. > + * > + * Combined, it means that the tracee owes a SIGCONT > + * notification to the real parent. > + */ > + if (sig->flags & SIGNAL_CLD_MASK) { > + sig->flags |= SIGNAL_NOTIFY_CONT;
Two threads, T1 and T2. T1 is ptraced, T2 is not.
SIGSTOP stops them both. T1 sleeps in TASK_TRACED, T2 in TASK_STOPPED.
prepare_signal(SIGCONT) sets SIGNAL_NOTIFY_CONT + SIGNAL_CLD_CONTINUED, and wakes T2 up.
T2 notifies its ->real_parent, clears SIGNAL_NOTIFY_CONT.
Debugger does ptrace(PTRACE_DETACH, T1), sees SIGNAL_CLD_MASK, and restores SIGNAL_NOTIFY_CONT.
T1 resends the (bogus) notification to its (and T2's) real_parent.
Even if I missed something,
> @@ -245,6 +273,14 @@ int ptrace_attach(struct task_struct *task) > signal_wake_up(task, 1); > } > > + /* > + * Clear SIGNAL_CLD_MASK if NOTIFY_CONT is not set. This is > + * used to preserve SIGCONT notification across ptrace > + * attach/detach. Read the comment in __ptrace_unlink(). > + */ > + if (!(task->signal->flags & SIGNAL_NOTIFY_CONT)) > + task->signal->flags &= ~SIGNAL_CLD_MASK;
What if there is another ptraced sub-thread in this group who "owes" the notification ?
> + * Force the tracee into signal delivery path so that > + * the notification is delievered ASAP. This wakeup > + * is unintrusive as SIGCONT delivery would have > + * caused the same effect. > + */ > + if (!woken_up) > + signal_wake_up(child, 0);
Well, signal_wake_up() can really force the tracee into signal delivery. It only sets TIF_SIGPENDING, but this can race with recalc_sigpending().
Oh. This reminds me: http://marc.info/?t=123411921400004
> @@ -1639,7 +1642,24 @@ static void do_notify_parent_cldstop(struct task_struct *tsk, int why) > > switch (why) { > case CLD_CONTINUED: > - notify = why; > + /* > + * Notify once if NOTIFY_CONT is set regardless of ptrace. > + * NOTIFY_CONT will be reinstated on detach if necessary. > + */ > + if (!(sig->flags & SIGNAL_NOTIFY_CONT)) > + break; > + > + /* > + * If ptraced, always report CLD_CONTINUED; otherwise, > + * prepare_signal(SIGCONT) encodes the CLD_ si_code into > + * SIGNAL_CLD_MASK bits. > + */ > + if (task_ptrace(tsk) || (sig->flags & SIGNAL_CLD_CONTINUED)) > + notify = CLD_CONTINUED;
See the comment on 4/16
> @@ -2015,31 +2035,18 @@ relock: > */ > try_to_freeze(); > > - spin_lock_irq(&sighand->siglock); > /* > - * Every stopped thread goes here after wakeup. Check to see if > - * we should notify the parent, prepare_signal(SIGCONT) encodes > - * the CLD_ si_code into SIGNAL_CLD_MASK bits. > + * Every stopped thread should go through this function after > + * waking up. Check to see if we should notify the parent. > */ > - if (unlikely(signal->flags & SIGNAL_CLD_MASK)) { > - int why; > - > - if (task_ptrace(current) || > - (signal->flags & SIGNAL_CLD_CONTINUED)) > - why = CLD_CONTINUED; > - else > - why = CLD_STOPPED; > - > - signal->flags &= ~SIGNAL_CLD_MASK; > - > - spin_unlock_irq(&sighand->siglock); > - > + if (unlikely(current->signal->flags & SIGNAL_NOTIFY_CONT)) {
I am not sure it is OK to check SIGNAL_NOTIFY_CONT without ->siglock. If we return from do_signal_stop(), everything is fine.
But if we got here because of __ptrace_unlink()->signal_wake_up(1), we can miss SIGNAL_NOTIFY_CONT.
Oleg.
| ![\](/images/icornerr.gif) |