Re: [PATCH UPDATED 8/9] ptrace: move JOBCTL_TRAPPING wait to wait(2) and ptrace_check_attach()

[Oleg Nesterov <oleg@redhat.com>]

On 05/14, Tejun Heo wrote:
>
> @@ -1409,15 +1409,29 @@ static int wait_task_stopped(struct wait
>  	if (!ptrace && !(wo->wo_flags & WUNTRACED))
>  		return 0;
>
> -	if (!task_stopped_code(p, ptrace))
> +	/*
> +	 * For ptrace waits, we can't reliably check whether wait condition
> +	 * exists without grabbing siglock due to JOBCTL_TRAPPING
> +	 * transitions.  A task might be temporarily in TASK_RUNNING while
> +	 * trapping which should be transparent to the ptracer.
> +	 *
> +	 * Note that we can avoid unconditionally grabbing siglock by
> +	 * wrapping TRAPPING test with two rmb's; however, let's stick with
> +	 * simpler implementation for now.
> +	 */
> +	if (!ptrace && !(p->signal->flags & SIGNAL_STOP_STOPPED))
>  		return 0;
>
>  	exit_code = 0;
>  	spin_lock_irq(&p->sighand->siglock);
>
>  	p_code = task_stopped_code(p, ptrace);
> -	if (unlikely(!p_code))
> +	if (unlikely(!p_code)) {
> +		/* if trapping, wait for it and restart the whole process */
> +		if (ptrace && ptrace_wait_trapping(p))
> +			return restart_syscall();

Hmm. I didn't even know we have restart_syscall()... It is a bit fragile,
it assumes recalc_sigpending() is not possible during return from syscall.
In particular this means recalc_sigpending() must not be called in irq.
OK, this seems to be true.

Anyway, restart_syscall() is not right for do_wait(), especially with the
next patch. If the caller was woken by the real signal which has a handler,
we should not restart without SA_RESTART.


It is very hard to review this series. Without the further changes, it is
not clear why do we need these preparations. IIUC, ptrace_wait_trapping()
is only needed because we are going to re-trap. Otherwise we could always
wait in ptrace_attach() afaics.

I am still worried we are loosing the tight control over JOBCTL_TRAPPING.
6/9 contributes to this too.

Oleg.