Messages in this thread | | | Date | Fri, 15 Apr 2022 14:56:12 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT |
| |
On 04/15, Peter Zijlstra wrote: > > On Fri, Apr 15, 2022 at 12:16:44PM +0200, Oleg Nesterov wrote: > > > > Lets forget about 3-5 which I didn't read carefully yet. So why do we > > need TRACED_FROZEN? > > The purpose of 1/5 was to not have any unique state in __state. To at > all times be able to reconstruct __state from outside information (where > needed). > > Agreed that this particular piece of state isn't needed until 5/5, but > the concept is independent (also 5/5 is insanely large already).
OK, so in my opinion it would be more clean if TRACED_FROZEN comes in a separate (and simple) patch before 5/5.
I won't really argue, but to me this flag looks confusing and unnecessary in 1-2 (which btw look like a -stable material to me).
> > Can't we simply change signal_wake_up_state(), > > > > void signal_wake_up_state(struct task_struct *t, unsigned int state) > > { > > set_tsk_thread_flag(t, TIF_SIGPENDING); > > /* > > * TASK_WAKEKILL also means wake it up in the stopped/traced/killable > > * case. We don't check t->state here because there is a race with it > > * executing another processor and just now entering stopped state. > > * By using wake_up_state, we ensure the process will wake up and > > * handle its death signal. > > */ > > if (wake_up_state(t, state | TASK_INTERRUPTIBLE)) > > t->jobctl &= ~(JOBCTL_STOPPED | JOBCTL_TRACED); > > else > > kick_process(t); > > } > > > > ? > > This would be broken when we so signal_wake_up_state() when state > doesn't match. Does that happen? I'm thikning siglock protects us from > the most obvious races, but still.
Yes, even set_tsk_thread_flag(TIF_SIGPENDING) is not safe without siglock.
> Also, signal_wake_up_state() really can do with that > lockdep_assert_held() as well ;-)
OK, lets add lockdep_assert_held() at the start of signal_wake_up_state ?
Agreed, this probably needs a comment, but this looks much simpler and more understandable than 2 additional "if (resume)" checks in signal_wake_up() and ptrace_signal_wake_up().
> > > > - spin_lock_irq(&task->sighand->siglock); > > > > if (task_is_traced(task) && !looks_like_a_spurious_pid(task) && > > > > !__fatal_signal_pending(task)) { > > > > task->jobctl |= JOBCTL_TRACED_FROZEN; > > > > WRITE_ONCE(task->__state, __TASK_TRACED); > > > > ret = true; > > > > } > > > > > > I would feel much better if this were still a task_func_call() > > > validating !->on_rq && !->on_cpu. > > > > Well, but "on_rq || on_cpu" would mean that wait_task_inactive() is buggy ? > > Yes, but I'm starting to feel a little paranoid here. Better safe than > sorry etc..
OK, can we simply add
WARN_ON_ONCE(ret && (on_rq || on_cpu));
after unlock_task_sighand() ? this is racy without rq_lock but should catch the possible problems.
> > - do you agree we can avoid JOBCTL_TRACED_FROZEN in 1-2 ? > > We can for the sake of 2 avoid TRACED_FROZEN, but as explained at the > start, the point of 1 was to ensure there is no unique state in __state, > and I think in that respect we can keep it, hmm?
See above... I understand the purpose of TRACED_FROZEN (I hope ;), but not in 1-2, and unless I missed something the change in signal_wake_up above simply looks better to me, but of course this is subjective.
> > - will you agree if I change ptrace_freeze_traced() to rely > > on __state == TASK_TRACED rather than task_is_traced() ? > > Yes.
Great, thanks. I'll return tomorrow.
Oleg.
| |