Messages in this thread Patch in this message | | | Date | Wed, 13 Apr 2022 18:58:17 +0200 | From | Peter Zijlstra <> | Subject | Re: [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:
| |