Re: [PATCH 7/7] ptrace: clean transitions between TASK_STOPPED and TRACED

[Tejun Heo <tj@kernel.org>]

Hello, Oleg.  Sorry about the delay.  I've been and still am
travelling and won't be very responsive until mid next week.

On Wed, Jan 05, 2011 at 05:35:42PM +0100, Oleg Nesterov wrote:
> To me, the whole series is fine.

Awesome.

> As for the user-visible changes, I believe they are carefully documented,
> hopefully Roland and Jan can take a look.

I think it would be a good idea to document the defined and probably
more importantly undefined aspects of the ptrace behavior somewhere
along with rational and implementation peculiarities.  Probably we
should create a file under Documentation and also make sure the ptrace
man page is kept synchronized with and point to it.

> This patch looks good too, a couple of minor nits below.
> 
> On 12/24, Tejun Heo wrote:
> >
> > + * task_clear_group_stop_trapping - clear group stop trapping bit
> > + * @task: target task
> > + *
> > + * If GROUP_STOP_TRAPPING is set, a ptracer is waiting for us.  Clear it
> > + * and wake up the ptracer.  Note that we don't need any further locking.
> > + * @task->siglock guarantees that @task->parent points to the ptracer.
> > + *
> > + * CONTEXT:
> > + * Must be called with @task->sighand->siglock held.
> > + */
> > +static void task_clear_group_stop_trapping(struct task_struct *task)
> > +{
> > +	if (unlikely(task->group_stop & GROUP_STOP_TRAPPING)) {
> > +		task->group_stop &= ~GROUP_STOP_TRAPPING;
> > +		__wake_up_sync(&task->parent->signal->wait_chldexit,
> > +			       TASK_UNINTERRUPTIBLE, 1);
> 
> OK... we are doing __wake_up_sync_key(key => NULL), this looks unfriendly
> to child_wait_callback(). But TASK_UNINTERRUPTIBLE means we can't abuse
> the tracer's subthreads doing do_wait().

Yeah, given the complexities around wait_chldexit, I'm not entirely
sure whether multiplexing it actually is a good idea.  It definitely
fits the purpose but I still feel dirty adding more subtleties to it.

> >  void task_clear_group_stop(struct task_struct *task)
> >  {
> >  	task->group_stop &= ~(GROUP_STOP_PENDING | GROUP_STOP_CONSUME);
> > +	task_clear_group_stop_trapping(task);
> >  }
> 
> Not a comment, but the question. I am not sure task_clear_group_stop()
> needs task_clear_group_stop_trapping(), please see below...

Hmm... I wanted to make sure that task_clear_group_stop() clears all
group stop related status.  As the function may be called from kill
path too, I wanted to make sure it is guaranteed to be cleared
together.  That was the rationale but maybe there's a better place for
it.

> > @@ -1694,6 +1716,14 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
> >  	}
> >  
> >  	/*
> > +	 * We're committing to trapping.  Clearing GROUP_STOP_TRAPPING and
> > +	 * transition to TASK_TRACED should be atomic with respect to
> > +	 * siglock.  Do it after the arch hook as siglock is released and
> > +	 * regrabbed across it.
> > +	 */
> > +	task_clear_group_stop_trapping(current);
> 
> This wakes up the tracer. It can return from sys_ptrace(), call do_wait(),
> and take tasklist_lock before us.
>
> Of course, this is only theoretical problem, but perhaps it makes sense
> to do this after __set_current_state(TASK_TRACED), otherwise
> task_stopped_code() can fail.

Right.  That's a slim possibility but definitely possible.

> > @@ -1839,13 +1875,25 @@ static int do_signal_stop(int signr)
> >  		schedule();
> >
> >  		spin_lock_irq(&current->sighand->siglock);
> > -	} else
> > -		ptrace_stop(current->exit_code, CLD_STOPPED, 0, NULL);
> > +	} else {
> > +		ptrace_stop(current->group_stop & GROUP_STOP_SIGMASK,
> > +			    CLD_STOPPED, 0, NULL);
> > +		current->exit_code = 0;
> > +	}
> > +
> > +	/*
> > +	 * GROUP_STOP_PENDING could be set if another group stop has
> > +	 * started since being woken up or ptrace wants us to transit
> > +	 * between TASK_STOPPED and TRACED.  Retry group stop.
> > +	 */
> > +	if (current->group_stop & GROUP_STOP_PENDING) {
> > +		WARN_ON_ONCE(!(current->group_stop & GROUP_STOP_SIGMASK));
> > +		goto retry;
> > +	}
> >
> >  	spin_unlock_irq(&current->sighand->siglock);
> 
> Can't we add task_clear_group_stop_trapping() right before we drop
> ->siglock ? This way we can remove it from task_clear_group_stop(),
> afaics. Once again, this is up to you. Looks more clean to me, but
> this is of course subjective.
> 
> If GROUP_STOP_PENDING is not set, but GROUP_STOP_TRAPPING is set,
> then this task was SIGKILL'ed or SIGCONT'ed, we can notify the
> tracer.
> 
> Otherwise (ignoring ptrace_stop), there is no reasons to check
> GROUP_STOP_TRAPPING. It was set under ->siglock when the tracee
> was in TASK_STOPPED state few lines above.

Hmm... I don't really mind one way or the other.  I like the idea that
clear_group_stop() clears everything but at the same time the
suggested placing makes it more explicit which is a plus.  I'll think
a bit more about it but if it doesnt break anything let's move it.

Thanks.

-- 
tejun