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

[Oleg Nesterov <oleg@redhat.com>]

To me, the whole series is fine.

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


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().

>  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...

> @@ -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.

> @@ -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.

Oleg.