Re: [PATCH v3 08/11] ptrace: Admit ptrace_stop can generate spuriuos SIGTRAPs

Oleg Nesterov
2022-05-05T14:57:22Z

On 05/04, Eric W. Biederman wrote:
>
> -static int ptrace_stop(int exit_code, int why, int clear_code,
> - unsigned long message, kernel_siginfo_t *info)
> +static int ptrace_stop(int exit_code, int why, unsigned long message,
> + kernel_siginfo_t *info)
> __releases(&current->sighand->siglock)
> __acquires(&current->sighand->siglock)
> {
> @@ -2259,54 +2259,33 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
>
> spin_unlock_irq(&current->sighand->siglock);
> read_lock(&tasklist_lock);
> - if (likely(current->ptrace)) {
> - /*
> - * Notify parents of the stop.
> - *
> - * While ptraced, there are two parents - the ptracer and
> - * the real_parent of the group_leader. The ptracer should
> - * know about every stop while the real parent is only
> - * interested in the completion of group stop. The states
> - * for the two don't interact with each other. Notify
> - * separately unless they're gonna be duplicates.
> - */
> + /*
> + * Notify parents of the stop.
> + *
> + * While ptraced, there are two parents - the ptracer and
> + * the real_parent of the group_leader. The ptracer should
> + * know about every stop while the real parent is only
> + * interested in the completion of group stop. The states
> + * for the two don't interact with each other. Notify
> + * separately unless they're gonna be duplicates.
> + */
> + if (current->ptrace)
> do_notify_parent_cldstop(current, true, why);
> - if (gstop_done && ptrace_reparented(current))
> - do_notify_parent_cldstop(current, false, why);
> + if (gstop_done && (!current->ptrace || ptrace_reparented(current)))
> + do_notify_parent_cldstop(current, false, why);
>
> - /*
> - * Don't want to allow preemption here, because
> - * sys_ptrace() needs this task to be inactive.
> - *
> - * XXX: implement read_unlock_no_resched().
> - */
> - preempt_disable();
> - read_unlock(&tasklist_lock);
> - cgroup_enter_frozen();
> - preempt_enable_no_resched();
> - freezable_schedule();
> - cgroup_leave_frozen(true);
> - } else {
> - /*
> - * By the time we got the lock, our tracer went away.
> - * Don't drop the lock yet, another tracer may come.
> - *
> - * If @gstop_done, the ptracer went away between group stop
> - * completion and here. During detach, it would have set
> - * JOBCTL_STOP_PENDING on us and we'll re-enter
> - * TASK_STOPPED in do_signal_stop() on return, so notifying
> - * the real parent of the group stop completion is enough.
> - */
> - if (gstop_done)
> - do_notify_parent_cldstop(current, false, why);
> -
> - /* tasklist protects us from ptrace_freeze_traced() */
> - __set_current_state(TASK_RUNNING);
> - read_code = false;
> - if (clear_code)
> - exit_code = 0;
> - read_unlock(&tasklist_lock);
> - }
> + /*
> + * Don't want to allow preemption here, because
> + * sys_ptrace() needs this task to be inactive.
> + *
> + * XXX: implement read_unlock_no_resched().
> + */
> + preempt_disable();
> + read_unlock(&tasklist_lock);
> + cgroup_enter_frozen();
> + preempt_enable_no_resched();
> + freezable_schedule();

I must have missed something.

So the tracee calls ptrace_notify() but debugger goes away before the
ptrace_notify() takes siglock. After that the no longer traced task
will sleep in TASK_TRACED ?

Looks like ptrace_stop() needs to check current->ptrace before it does
set_special_state(TASK_TRACED) with siglock held? Then we can rely on
ptrace_unlink() which will wake the tracee up even if debugger exits.

No?

Oleg.