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

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Dec 22, 2010 at 12:54:09PM +0100, Oleg Nesterov wrote:
> On 12/21, Tejun Heo wrote:
> > 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.
> 
> Agreed. And the code itself certainly becomes correct/consistent,
> contrary to "everything is broken" we currently have.
> 
> Tejun, I'll try to summarize my (very foggy) concerns in a separate
> email. Don't get me wrong, I think this series rightly addresses the
> numerous problems we have. My only question is, can't we go a bit
> further and create the new (and simple) rules. Probably not.

Yeah, definitely, if we're gonna make some userland visible changes,
let's get it right once and for all (at least in terms of the intended
behaviors, that is).  It's sure gonna take more work but I think it'll
be manageable both in terms of the required effort and implementation
complexity.  Well, actually, in terms of the latter, I think we're
likely to improve the situation by making the rules and intentions
clear.

> > > > @@ -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.
> 
> Sure, but is is not held after we drop ->siglock. And ptrace_attach() can
> happen in the window before we take it for do_notify_parent_cldstop().

I thought the code snippet was from inside do_notify_parent_cldstop()
for some reason.  Okay, so the debugger can attach there
and... hmmm... right.  Yeah, the debugger gets the extra notification.
I don't think the previous code fared any better tho.  Anyways, I'll
think about how to fix this.

Thanks.

-- 
tejun