Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 21 Apr 2022 21:58:52 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     rjw@rjwysocki.net, mingo@kernel.org, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, mgorman@suse.de,
        ebiederm@xmission.com, bigeasy@linutronix.de,
        Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org,
        tj@kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v2 2/5] sched,ptrace: Fix ptrace_check_attach() vs
 PREEMPT_RT
Message-ID: <20220421195852.GP2731@worktop.programming.kicks-ass.net>
References: <20220421150248.667412396@infradead.org>
 <20220421150654.817117821@infradead.org>
 <20220421182325.GC20402@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220421182325.GC20402@redhat.com>
Precedence: bulk

On Thu, Apr 21, 2022 at 08:23:26PM +0200, Oleg Nesterov wrote:
> On 04/21, Peter Zijlstra wrote:
> >
> > Rework ptrace_check_attach() / ptrace_unfreeze_traced() to not rely on
> > task->__state as much.
> 
> Looks good after the quick glance... but to me honest I got lost and
> I need to apply these patches and read the code carefully.
> 
> However, I am not able to do this until Monday, sorry.

Sure, no worries. Take your time.

> Just one nit for now,
> 
> >  static void ptrace_unfreeze_traced(struct task_struct *task)
> >  {
> > -	if (READ_ONCE(task->__state) != __TASK_TRACED)
> > +	if (!task_is_traced(task))
> >  		return;
> >  
> >  	WARN_ON(!task->ptrace || task->parent != current);
> >  
> > -	/*
> > -	 * PTRACE_LISTEN can allow ptrace_trap_notify to wake us up remotely.
> > -	 * Recheck state under the lock to close this race.
> > -	 */
> >  	spin_lock_irq(&task->sighand->siglock);
> > -	if (READ_ONCE(task->__state) == __TASK_TRACED) {
> > +	if (task_is_traced(task)) {
> 
> I think ptrace_unfreeze_traced() should not use task_is_traced() at all.
> I think a single lockless
> 
> 	if (task->jobctl & JOBCTL_DELAY_WAKEKILL)
> 		return;
> 
> at the start should be enough?

I think so. That is indeed cleaner. I'll make the change if I don't see
anything wrong with it in the morning when the brain has woken up again
;-)

> 
> Nobody else can set this flag. It can be cleared by the tracee if it was
> woken up, so perhaps we can check it again but afaics this is not strictly
> needed.
> 
> > +//		WARN_ON_ONCE(!(task->jobctl & JOBCTL_DELAY_WAKEKILL));
> 
> Did you really want to add the commented WARN_ON_ONCE?

I did that because:

@@ -1472,8 +1479,7 @@ COMPAT_SYSCALL_DEFINE4(ptrace, compat_lo
                                  request == PTRACE_INTERRUPT);
        if (!ret) {
                ret = compat_arch_ptrace(child, request, addr, data);
-               if (ret || request != PTRACE_DETACH)
-                       ptrace_unfreeze_traced(child);
+               ptrace_unfreeze_traced(child);
        }

Can now call unfreeze too often. I left the comment in because I need to
think more about why Eric did that and see if it really is needed.