Oleg Nesterov <o...@redhat.com> writes:
> (add akpm, we usually route ptrace fixes via -mm tree)
>
> On 02/21, bseg...@google.com wrote:
>>
>> --- a/kernel/ptrace.c
>> +++ b/kernel/ptrace.c
>> @@ -184,10 +184,14 @@ static void ptrace_unfreeze_traced(struct task_struct
>> *task)
>>
>> WARN_ON(!task->ptrace || task->parent != current);
>>
>> + /*
>> + * Double check __TASK_TRACED under the lock to prevent corrupting
>> state
>> + * in case of a ptrace_trap_notify wakeup
>> + */
>> spin_lock_irq(&task->sighand->siglock);
>> if (__fatal_signal_pending(task))
>> wake_up_state(task, __TASK_TRACED);
>> - else
>> + else if (task->state == __TASK_TRACED)
>> task->state = TASK_TRACED;
>> spin_unlock_irq(&task->sighand->siglock);
>
> So yes, I think your patch is fine except the comment should explain that
> we need this because PTRACE_LISTEN makes ptrace_trap_notify() possible. And
> perhaps it would be better to do the 2nd check before fatal_signal_pending:
>
> if (task->state == __TASK_TRACED) {
> if (__fatal_signal_pending(task))
> wake_up_state(task, __TASK_TRACED);
> else
> task->state = TASK_TRACED;
> }
>
> just to make the logic more clear. wake_up_state(__TASK_TRACED) can
> never hurt if the task is killed, just it doesn't look strictly correct
> if the tracee was already woken. But this is minor.
>
>
>
> You know, I'd prefer another fix, see below.
>
> Why. ptrace_unfreeze_traced() assumes that - since ptrace_freeze_traced()
> checks PTRACE_LISTEN - nobody but us can wake the tracee up. So the
> __TASK_TRACED check at the start of ptrace_unfreeze_traced() means that
> the tracee is still freezed, it was not woken up by (say) PTRACE_CONT.
>
> IOW, currently we assume that only the caller of ptrace_freeze_traced()
> can do the __TASK_TRACED -> WHATEVER transition.
>
> However, as you pointed out, I forgot that JOBCTL_LISTENING set by LISTEN
> breaks this assumption, and imo it would be nice to fix this.
>
> What do you think? I won't insist too much if you prefer your simple
> change.
My knowledge of the ptrace state machine isn't the best, but this looks
valid to me and doesn't crash
>
> Oleg.
>
> --- x/kernel/ptrace.c
> +++ x/kernel/ptrace.c
> @@ -174,6 +174,18 @@
> return ret;
> }
>
> +static bool __ptrace_unfreeze_traced(struct task_struct *task)
> +{
> + bool killed = __fatal_signal_pending(task);
> +
> + if (killed)
> + wake_up_state(task, __TASK_TRACED);
> + else
> + task->state = TASK_TRACED;
> +
> + return !killed'
> +}
> +
> static void ptrace_unfreeze_traced(struct task_struct *task)
> {
> if (task->state != __TASK_TRACED)
> @@ -182,10 +194,7 @@
> WARN_ON(!task->ptrace || task->parent != current);
>
> spin_lock_irq(&task->sighand->siglock);
> - if (__fatal_signal_pending(task))
> - wake_up_state(task, __TASK_TRACED);
> - else
> - task->state = TASK_TRACED;
> + __ptrace_unfreeze_traced(task);
> spin_unlock_irq(&task->sighand->siglock);
> }
>
> @@ -993,7 +1002,12 @@
> break;
>
> si = child->last_siginfo;
> - if (likely(si && (si->si_code >> 8) == PTRACE_EVENT_STOP)) {
> + /*
> + * Once we set JOBCTL_LISTENING we do not own child->state,
> + * need to unfreeze first.
> + */
> + if (__ptrace_unfreeze_traced(child) &&
> + likely(si && (si->si_code >> 8) == PTRACE_EVENT_STOP)) {
> child->jobctl |= JOBCTL_LISTENING;
> /*
> * If NOTIFY is set, it means event happened between
