From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751692AbdB0SIp (ORCPT ); Mon, 27 Feb 2017 13:08:45 -0500 Received: from mail-pg0-f53.google.com ([74.125.83.53]:36039 "EHLO mail-pg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751173AbdB0SI1 (ORCPT ); Mon, 27 Feb 2017 13:08:27 -0500 From: bsegall@google.com To: Oleg Nesterov Cc: linux-kernel@vger.kernel.org, Roland McGrath , Andrew Morton , Tejun Heo Subject: Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state References: <20170224163611.GA24902@redhat.com> Date: Mon, 27 Feb 2017 10:08:23 -0800 In-Reply-To: <20170224163611.GA24902@redhat.com> (Oleg Nesterov's message of "Fri, 24 Feb 2017 17:36:11 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Oleg Nesterov writes: > (add akpm, we usually route ptrace fixes via -mm tree) > > On 02/21, bsegall@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