On 2020-11-02 16:50:01 [+0100], Oleg Nesterov wrote:
> The commit 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")
> changed ptrace_freeze_traced() to take task->saved_state into account,
> but ptrace_unfreeze_traced() has the same problem and needs a similar
> fix: it should check/update both ->state and ->saved_state.
Thank you for the report.
> Reported-by: Luis Claudio R. Goncalves <lgoncalv@xxxxxxxxxx>
> Fixes: 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
> kernel/ptrace.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 3075006d720e..7e004f998233 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -197,8 +197,7 @@ static bool ptrace_freeze_traced(struct task_struct *task)
>
> static void ptrace_unfreeze_traced(struct task_struct *task)
> {
> - if (task->state != __TASK_TRACED)
> - return;
> + bool frozen = true;
Is this okay for !PREEMPT_RT or is this considered as an important
fast-path on !PREEMPT_RT?
> WARN_ON(!task->ptrace || task->parent != current);
>
> @@ -207,12 +206,19 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
> * Recheck state under the lock to close this race.
> */
> spin_lock_irq(&task->sighand->siglock);
> - if (task->state == __TASK_TRACED) {
> - if (__fatal_signal_pending(task))
> - wake_up_state(task, __TASK_TRACED);
> - else
> - task->state = TASK_TRACED;
> - }
> +
> + raw_spin_lock(&task->pi_lock);
I think this got mentioned in the other emails but this requires irqsave
because the above (siglock) does not disable interrupts on RT.
This `saved_state' is only used for RT so it might make sense to avoid
the PI-lock on non-RT.
> + if (task->state == __TASK_TRACED)
> + task->state = TASK_TRACED;
> + else if (task->saved_state == __TASK_TRACED)
> + task->saved_state = TASK_TRACED;
> + else
> + frozen = false;
> + raw_spin_unlock(&task->pi_lock);
> +
> + if (frozen && __fatal_signal_pending(task))
> + wake_up_state(task, __TASK_TRACED);
> +
> spin_unlock_irq(&task->sighand->siglock);
> }
>
Sebastian
