lkml.org 
[lkml]   [2022]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2022-04-21 22:00    [W:0.543 / U:0.396 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site