lkml.org 
[lkml]   [2022]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT
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:
\
 
 \ /
  Last update: 2022-04-13 18:59    [W:0.101 / U:0.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site