Messages in this thread | | | Date | Mon, 11 Apr 2022 19:07:53 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v2] ptrace: fix ptrace vs tasklist_lock race on PREEMPT_RT. |
| |
On Mon, Apr 11, 2022 at 08:44:24AM -0500, Eric W. Biederman wrote: > Peter Zijlstra <peterz@infradead.org> writes: > > > On Fri, Apr 08, 2022 at 10:06:30PM +0200, Peter Zijlstra wrote: > > > >> I'll ponder if wait_task_inactive() can simplify things.. > > > > This,.. so ptrace_check_attach(), which does ptrace_freeze_traced() > > already does wait_task_inactive(), but on the 'wrong' side of things. > > > > AFAICT, if we move that up, we're almost there, except that opens up a > > detach+attach race. That could be fixed by doing another > > wait_task_inactive(), but we can't due to locking :/ > > > > Let's see if I can make that work without making a mess of things. > > Because ensuring the task is stuck in schedule() makes the whole > > saved_state thing go away -- as you noted. > > The code can perhaps synchronize on a bit using the the full locking and > then drop the locks and call the wait_task_inactive or whatever. > > The challenge as I see it is after the traced task is inactive to allow > "wake_up_state(t, TASK_WAKEKILL)" on the traced task, have the traced > tasks state change to TASK_RUNNING and not allow the traced task to run > until what is today ptrace_unfreeze_task is called. > > I just don't know how to get something stuck and not allow it to run.
Same as today? Clear TASK_WAKEKILL from __state and check __fatal_signal_pending() before putting it back again.
The thing is, once we hit schedule() with TASK_TRACED, there's only two possible ways for that task to wake up:
wake_up_state(t, TASK_WAKEKILL)
and
wake_up_state(t, __TASK_TRACED)
both are issued while holding sighand lock, so provided we hold sighand lock, we can actually frob __state as we do today, we just need to know the task really has scheduled out first.
That is, the problem today, for PREEMPT_RT, is:
ptrace_stop(): ptrace_freeze_traced()
set_special_state(TASK_TRACING)
...
spin_lock(&foo) current->saved_state = current->__state; current->__state = TASK_RTLOCK_WAIT
READ_ONCE(t->__state) // whoopsie, not // TRACED
...
schedule()
But if wait_task_inactive() ensures our @t is actually in schedule(), we're good again, nothing will then ever change __state as long as we hold sighand lock.
The only fun bit is that wait_task_inactive() likes to schedule so we want do that with sighand lock held. What we need to do is call it first, and then re-check stuff is still sane once we (re)acquire all the locks.
This is certainly possible -- and not in fact too hard; the only thing I'm really concerned about is not making it more ugly than dealing with saved_state in the first place (and *that* is turning out to be somewhat hard).
But while going over all this I think there might be an additional problem; wait_task_inactive() is stubbed for SMP=n...
| |