Messages in this thread Patch in this message | | | Date | Thu, 21 Apr 2022 09:21:38 +0200 | From | Peter Zijlstra <> | Subject | Re: [RFC][PATCH] ptrace: Don't change __state |
| |
On Wed, Apr 20, 2022 at 03:54:15PM -0500, 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 would be a good technique to completely decouple > PREEMPT_RT from the work that ptrace_freeze_attach does. > > Comments?
On first read-through, I like it! A few comments down below..
> @@ -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);
^^^^ this should be spin_lock_irq(...)
> + WARN_ON(!(task->jobctl & JOBCTL_DELAY_WAKEKILL)); > + task->jobctl &= ~JOBCTL_DELAY_WAKEKILL; > + if (fatal_signal_pending(task)) > + wake_up_state(task, TASK_WAKEKILL); > spin_unlock_irq(&task->sighand->siglock); > } > > @@ -256,7 +255,7 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state) > */ > read_lock(&tasklist_lock); > if (child->ptrace && child->parent == current) { > - WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED); > + WARN_ON(child->jobctl & JOBCTL_DELAY_WAKEKILL); > /* > * child->sighand can't be NULL, release_task() > * does ptrace_unlink() before __exit_signal(). > @@ -267,13 +266,13 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state) > read_unlock(&tasklist_lock); > > if (!ret && !ignore_state) { > - if (!wait_task_inactive(child, __TASK_TRACED)) { > + if (!wait_task_inactive(child, TASK_TRACED)) {
This is still very dubious, there are spinlocks between set_current_state(TASK_TRACED) and schedule(), so wait_task_inactive() can fail where we don't want it to due to TASK_TRACED being temporarily held in ->saved_state.
> /* > * This can only happen if may_ptrace_stop() fails and > * ptrace_stop() changes ->state back to TASK_RUNNING, > - * so we should not worry about leaking __TASK_TRACED. > + * so we should not worry about leaking JOBCTL_DELAY_WAKEKILL. > */ > + WARN_ON(!(child->jobctl & JOBCTL_DELAY_WAKEKILL)); > ret = -ESRCH; > } > }
| |