Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Apr 2022 18:58:17 +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 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT
Message-ID: <YlcBKQvp524p0KRY@hirez.programming.kicks-ass.net>
References: <20220412114421.691372568@infradead.org>
 <20220412114853.842942162@infradead.org>
 <20220413132451.GA27281@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220413132451.GA27281@redhat.com>
Precedence: bulk

On Wed, Apr 13, 2022 at 03:24:52PM +0200, Oleg Nesterov wrote:
> Hi Peter,
> 
> I like 1-2 but I need to read them (and other patches) again, a
> couple of nits right now.
> 
> On 04/12, Peter Zijlstra wrote:
> >
> > +static int __ptrace_freeze_cond(struct task_struct *p)
> > +{
> > +	if (!task_is_traced(p))
> > +		return -ESRCH;
> 
> 	if (!task_is_traced(p) || p->parent != current)
> 		return -ESRCH;
> 
> we should not spin/sleep if it is traced by another task

Yes, fair enough. And I suppose doing this test without holding siglock
is safe enough.

> > +static int __ptrace_freeze(struct task_struct *p, void *arg)
> > +{
> > +	int ret;
> > +
> > +	ret = __ptrace_freeze_cond(p);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Task scheduled between __ptrace_pre_freeze() and here, not our task
> > +	 * anymore.
> > +	 */
> > +	if (*(unsigned long *)arg != p->nvcsw)
> > +		return -ESRCH;
> > +
> > +	if (looks_like_a_spurious_pid(p))
> > +		return -ESRCH;
> 
> Oh, I do not think __ptrace_freeze() should check for spurious pid...
> looks_like_a_spurious_pid() should be called once in ptrace_check_attach()
> before task_call_func(__ptrace_freeze).

I can certainly do that, but since that needs be done with siglock held,
and the __ptrace_freeze call is a one-time affair, I didn't really see
the point in making the code more complicated.

Something like so then?

--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -222,7 +222,7 @@ static void ptrace_unfreeze_traced(struc
  */
 static int __ptrace_freeze_cond(struct task_struct *p)
 {
-	if (!task_is_traced(p))
+	if (!task_is_traced(p) || p->parent != current)
 		return -ESRCH;
 
 	if (task_curr(p))
@@ -283,9 +283,6 @@ static int __ptrace_freeze(struct task_s
 	if (*(unsigned long *)arg != p->nvcsw)
 		return -ESRCH;
 
-	if (looks_like_a_spurious_pid(p))
-		return -ESRCH;
-
 	if (__fatal_signal_pending(p))
 		return -ESRCH;
 
@@ -378,6 +375,9 @@ static int ptrace_check_attach(struct ta
 		 * does ptrace_unlink() before __exit_signal().
 		 */
 		spin_lock_irq(&child->sighand->siglock);
+		if (looks_like_a_spurious_pid(child))
+			goto unlock_sig;
+
 		ret = task_call_func(child, __ptrace_freeze, &nvcsw);
 		if (ret) {
 			/*
@@ -386,6 +386,7 @@ static int ptrace_check_attach(struct ta
 			 */
 			ret = -ESRCH;
 		}
+unlock_sig:
 		spin_unlock_irq(&child->sighand->siglock);
 	}
 unlock: