Messages in this thread | | | Date | Thu, 21 Apr 2022 21:58:52 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v2 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT |
| |
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.
| |