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

Tejun Heo
2011-01-09T22:05:10Z

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