From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751318AbdBXQjf (ORCPT ); Fri, 24 Feb 2017 11:39:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33544 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751163AbdBXQj1 (ORCPT ); Fri, 24 Feb 2017 11:39:27 -0500 Date: Fri, 24 Feb 2017 17:36:11 +0100 From: Oleg Nesterov To: bsegall@google.com Cc: linux-kernel@vger.kernel.org, Roland McGrath , Andrew Morton , Tejun Heo Subject: Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state Message-ID: <20170224163611.GA24902@redhat.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 24 Feb 2017 16:37:50 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (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. 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