Re: [PATCH 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT

[Oleg Nesterov]

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.