Re: [PATCH ptrace] ptrace: use GROUP_STOP_TRAPPING for PTRACE_DETACH too

[Tejun Heo <tj@kernel.org>]

Hey, Oleg.

On Sun, May 08, 2011 at 06:07:20PM +0200, Oleg Nesterov wrote:
> > 	  for (i = 0; i < 100000; i++) {
> > 		  ptrace(PTRACE_ATTACH, tracee, NULL, NULL);
> > 		  waitid(P_PID, tracee, &si, WSTOPPED | WNOHANG);
> > 		  if (!si.si_pid)
> > 			  nr_wait_fails++;
> 
> OK, this is clear, waitid(WSTOPPED | WNOHANG) can fail if it sees the
> tracee inside the transition.
> 
> But,
> 
> > 		  if (ptrace(PTRACE_DETACH, tracee, NULL, NULL)) {
> > 			  nr_ptrace_fails++;
> 
> I assume this can only fail for the same reason if waitid() fails?
> Or there is something else?

Oh yeah, I was just curious about the timing and see how many fail
waitid() but succeed ptrace().

> >  	/*
> >  	 * Reinstate GROUP_STOP_PENDING if group stop is in effect and
> > -	 * @child isn't dead.
> > +	 * @child isn't dead.  This will trigger TRACED -> RUNNING ->
> > +	 * STOPPED transition.  As this transition can affect the next
> > +	 * ptracer if it attaches before the transition completes, set
> > +	 * TRAPPING too.  Read comment in ptrace_attach() for more details.
> >  	 */
> >  	if (!(child->flags & PF_EXITING) &&
> >  	    (child->signal->flags & SIGNAL_STOP_STOPPED ||
> >  	     child->signal->group_stop_count))
> > -		child->group_stop |= GROUP_STOP_PENDING;
> > +		child->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
> 
> This doesn't look safe, see below. We do not know what the tracee does,
> it can be even running.

Ah, right.  Setting TRAPPING should probably depend on JOBCTL_TRAPPED
flag which is added later.

> >  static int ptrace_attach(struct task_struct *task)
> >  {
> > -	bool wait_trap = false;
> >  	int retval;
> >
> >  	audit_ptrace(task);
> > @@ -245,7 +247,6 @@ static int ptrace_attach(struct task_str
> >  	if (task_is_stopped(task)) {
> >  		task->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
> >  		signal_wake_up(task, 1);
> > -		wait_trap = true;
> >  	}
> >
> >  	spin_unlock(&task->sighand->siglock);
> > @@ -256,9 +257,8 @@ unlock_tasklist:
> >  unlock_creds:
> >  	mutex_unlock(&task->signal->cred_guard_mutex);
> >  out:
> > -	if (wait_trap)
> > -		wait_event(current->signal->wait_chldexit,
> > -			   !(task->group_stop & GROUP_STOP_TRAPPING));
> > +	wait_event(current->signal->wait_chldexit,
> > +		   !(task->group_stop & GROUP_STOP_TRAPPING));
> 
> Suppose that SIGCONT or, worse, SIGKILL comes in between.

Please ignore this one for now.  I'll re-do it after the SEIZE series.

Thanks.

-- 
tejun