Messages in this thread | | | Date | Thu, 21 Apr 2022 11:46:41 +0200 | From | Oleg Nesterov <> | Subject | Re: [RFC][PATCH] ptrace: Don't change __state |
| |
On 04/20, Eric W. Biederman wrote: > > I was thinking about this and I have an approach from a different > direction. In particular it removes the need for ptrace_freeze_attach > and ptrace_unfreeze_attach to change __state. Instead a jobctl > bit is used to suppress waking up a process with TASK_WAKEKILL.
I think this can work, but we still need something like 1/5 + 2/5?
> I think this would be a good technique to completely decouple > PREEMPT_RT from the work that ptrace_freeze_attach does.
If CONFIG_RT=y we can't rely on the ->__state check in task_is_traced(), and wait_task_inactive() can wrongly fail if the tracee sleeps waiting for tasklist_lock.
A couple of comments after a quick glance,
> 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); > @@ -216,13 +217,11 @@ static void ptrace_unfreeze_traced(struct task_struct *task) > * 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 (__fatal_signal_pending(task)) > - wake_up_state(task, __TASK_TRACED); > - else > - WRITE_ONCE(task->__state, TASK_TRACED); > - } > + spin_unlock_irq(&task->sighand->siglock); > + WARN_ON(!(task->jobctl & JOBCTL_DELAY_WAKEKILL)); > + task->jobctl &= ~JOBCTL_DELAY_WAKEKILL;
We can't rely on the lockless task_is_traced() check above... probably this is fine, but I need to re-chesk. But at least you need to remove the comment about PTRACE_LISTEN above.
Another problem is that WARN_ON(!(task->jobctl & JOBCTL_DELAY_WAKEKILL)) doesn't look right if ignore_state in ptrace_check_attach() was true, the tracee could stop before ptrace_unfreeze_traced().
> @@ -892,7 +891,6 @@ static int ptrace_resume(struct task_struct *child, long request, > * status and clears the code too; this can't race with the tracee, it > * takes siglock after resume. > */ > - need_siglock = data && !thread_group_empty(current); > if (need_siglock) > spin_lock_irq(&child->sighand->siglock);
Hmm?
Oleg.
| |