Re: [PATCH 12/16] ptrace: make group stop notification reliable against ptrace

[Tejun Heo <tj@kernel.org>]

On Mon, Dec 20, 2010 at 06:34:25PM +0100, Oleg Nesterov wrote:
> On 12/06, Tejun Heo wrote:
> >
> > This patch adds a new signal flag SIGNAL_NOTIFY_STOP which is set on
> > group stop completion and cleared on notification to the real parent
> > or together with other stopped flags on SIGCONT/KILL.  This guarantees
> > that the real parent is notified correctly regardless of ptrace.
> 
> OK, I am a bit confused. I do not understand exactly what this
> "correctly" actually means.

It means that the ptracer wouldn't eat the notification.  The
notification is buffered and delivered when ptrace detaches.

> > If a ptraced task is the last task to stop, the notification is
> > postponed till ptrace detach or canceled if SIGCONT/KILL is
> > received inbetween.
> 
> OK. But what if the last task to stop is not ptraced? In this case
> ->real_parent gets the notification.
> 
> Of course, the current behaviour is not better, it is obviously wrong.
> But if we want to fix things, perhaps we should invite the new and
> clear rules. Isn't it better to always notify ->real_parent when the
> last thread stops in STOPPED/TRACED state? Otherwise the behaviour is
> not predictable, it depends on task_ptrace() state of the last thead
> which sees SIGNAL_NOTIFY_STOP.

I see.  My focus was to make ptrace attach/detach transparent.  IOW,
minimizing the effect of a debugger (or gcore or whatever) attaching
and then leaving.  So, this patch just makes sure that the
notification isn't absorbed by a ptracer.

> Actually, I think it would be even better to never notify ->real_parent
> until debugger detaches all threads, but this is not simple to implement.

But, yes, this is gonna change the behavior in subtle ways anyway so
it would be better to take care of that too.

> > @@ -1901,21 +1925,12 @@ retry:
> >  	__set_current_state(TASK_STOPPED);
> >
> >  	if (likely(!task_ptrace(current))) {
> > -		int notify = 0;
> > -
> > -		/*
> > -		 * If there are no other threads in the group, or if there
> > -		 * is a group stop in progress and we are the last to stop,
> > -		 * report to the parent.
> > -		 */
> > -		if (task_participate_group_stop(current))
> > -			notify = CLD_STOPPED;
> > -
> > +		task_participate_group_stop(current);
> >  		spin_unlock_irq(&current->sighand->siglock);
> >
> > -		if (notify) {
> > +		if (sig->flags & SIGNAL_NOTIFY_STOP) {
> >  			read_lock(&tasklist_lock);
> > -			do_notify_parent_cldstop(current, notify);
> > +			do_notify_parent_cldstop(current, CLD_STOPPED);
> 
> Suppose that debugger attaches right after spin_unlock(->siglock).
> 
> Nothing really bad can happen afaics, but in this case the debugger
> will be notified twice. Hmm. If the debugger does do_wait() immediately
> after the first notification, it has all rights to see the stopped
> tracee but wait_task_stopped() fails, not good.

Hmmm?  ptrace_attach() can't happen while tasklist_lock is held.

Thanks.

-- 
tejun