Messages in this thread | | | From | "Eric W. Biederman" <> | Date | Thu, 21 Apr 2022 09:45:54 -0500 | Subject | Re: [RFC][PATCH] ptrace: Don't change __state |
| |
Peter Zijlstra <peterz@infradead.org> writes:
> 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(...)
Doh!
Thank you for spotting that. That solves my nasty splat in __send_signal.
> >> + 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.
When it comes to PREEMPT_RT yes.
I think we might be able to remove the wait_task_inactive, I am not certain what it gets us.
All this change gets us is the removal of playing with __state.
Eric
| |