Message-ID: <87tua5b193.fsf@email.froward.int.ebiederm.org>
Date:   Wed, 04 May 2022 13:28:56 -0500
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     linux-kernel@...r.kernel.org, rjw@...ysocki.net, mingo@...nel.org,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, mgorman@...e.de, bigeasy@...utronix.de,
        Will Deacon <will@...nel.org>, tj@...nel.org,
        linux-pm@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
        Richard Weinberger <richard@....at>,
        Anton Ivanov <anton.ivanov@...bridgegreys.com>,
        Johannes Berg <johannes@...solutions.net>,
        linux-um@...ts.infradead.org, Chris Zankel <chris@...kel.net>,
        Max Filippov <jcmvbkbc@...il.com>,
        linux-xtensa@...ux-xtensa.org, Kees Cook <keescook@...omium.org>,
        Jann Horn <jannh@...gle.com>, linux-ia64@...r.kernel.org
Subject: Re: [PATCH v2 07/12] ptrace: Don't change __state

"Eric W. Biederman" <ebiederm@...ssion.com> writes:

> Oleg Nesterov <oleg@...hat.com> writes:
>
>> On 05/03, Eric W. Biederman wrote:
>>>
>>> Oleg Nesterov <oleg@...hat.com> writes:
>>>
>>> > But why is it bad if the tracee doesn't sleep in schedule ? If it races
>>> > with SIGKILL. I still can't understand this.
>>> >
>>> > Yes, wait_task_inactive() can fail, so you need to remove WARN_ON_ONCE()
>>> > in 11/12.
>>>
>>> >
>>> > Why is removing TASK_WAKEKILL from TASK_TRACED and complicating
>>> > *signal_wake_up() better?
>>>
>>> Not changing __state is better because it removes special cases
>>> from the scheduler that only apply to ptrace.
>>
>> Hmm. But I didn't argue with that? I like the idea of JOBCTL_TASK_FROZEN.
>>
>> I meant, I do not think that removing KILLABLE from TASK_TRACED (not
>> from __state) and complicating *signal_wake_up() (I mean, compared
>> to your previous version) is a good idea.
>>
>> And. At least in context of this series it is fine if the JOBCTL_TASK_FROZEN
>> tracee do not block in schedule(), just you need to remove WARN_ON_ONCE()
>> around wait_task_inactive().
>>
>>> > And even if we need to ensure the tracee will always block after
>>> > ptrace_freeze_traced(), we can change signal_pending_state() to
>>> > return false if JOBCTL_PTRACE_FROZEN. Much simpler, imo. But still
>>> > looks unnecessary to me.
>>>
>>> We still need to change signal_wake_up in that case.  Possibly
>>> signal_wake_up_state.
>>
>> Of course. See above.
>>
>>> >> if we depend on wait_task_inactive failing if the process is in the
>>> >> wrong state.
>>> >
>>> > OK, I guess this is what I do not understand. Could you spell please?
>>> >
>>> > And speaking of RT, wait_task_inactive() still can fail because
>>> > cgroup_enter_frozen() takes css_set_lock? And it is called under
>>> > preempt_disable() ? I don't understand the plan :/
>>>
>>> Let me describe his freezer change as that is much easier to get to the
>>> final result.  RT has more problems as it turns all spin locks into
>>> sleeping locks.  When a task is frozen
>>
>> [...snip...]
>>
>> Oh, thanks Eric, but I understand this part. But I still can't understand
>> why is it that critical to block in schedule... OK, I need to think about
>> it. Lets assume this is really necessary.
>>
>> Anyway. I'd suggest to not change TASK_TRACED in this series and not
>> complicate signal_wake_up() more than you did in your previous version:
>>
>> 	static inline void signal_wake_up(struct task_struct *t, bool resume)
>> 	{
>> 		bool wakekill = resume && !(t->jobctl & JOBCTL_DELAY_WAKEKILL);
>> 		signal_wake_up_state(t, wakekill ? TASK_WAKEKILL : 0);
>> 	}
>
> If your concern is signal_wake_up there is no reason it can't be:
>
> 	static inline void signal_wake_up(struct task_struct *t, bool fatal)
>         {
>         	fatal = fatal && !(t->jobctl & JOBCTL_PTRACE_FROZEN);
>                 signal_wake_up_state(t, fatal ? TASK_WAKEKILL | TASK_TRACED : 0);
>         }
>
> I guess I was more targeted in this version, which lead to more if
> statements but as there is only one place in the code that can be
> JOBCTL_PTRACE_FROZEN and TASK_TRACED there is no point in setting
> TASK_WAKEKILL without also setting TASK_TRACED in the wake-up.
>
> So yes. I can make the code as simple as my earlier version of
> signal_wake_up.
>
>> JOBCTL_PTRACE_FROZEN is fine.
>>
>> ptrace_check_attach() can do
>>
>> 	if (!ret && !ignore_state &&
>> 	    /*
>> 	     * This can only fail if the frozen tracee races with
>> 	     * SIGKILL and enters schedule() with fatal_signal_pending
>> 	     */
>> 	    !wait_task_inactive(child, __TASK_TRACED))
>> 		ret = -ESRCH;
>>
>> 	return ret;
>>
>>
>> Now. If/when we really need to ensure that the frozen tracee always
>> blocks and wait_task_inactive() never fails, we can just do
>>
>> 	- add the fatal_signal_pending() check into ptrace_stop()
>> 	  (like this patch does)
>>
>> 	- say, change signal_pending_state:
>>
>> 	static inline int signal_pending_state(unsigned int state, struct task_struct *p)
>> 	{
>> 		if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
>> 			return 0;
>> 		if (!signal_pending(p))
>> 			return 0;
>> 		if (p->jobctl & JOBCTL_TASK_FROZEN)
>> 			return 0;
>> 		return (state & TASK_INTERRUPTIBLE) || __fatal_signal_pending(p);
>> 	}
>>
>> in a separate patch which should carefully document the need for this
>> change.
>>
>>> > I didn't look at JOBCTL_PTRACE_SIGNR yet. But this looks minor to me,
>>> > I mean, I am not sure it worth the trouble.
>>>
>>> The immediate problem the JOBCTL_PTRACE_SIGNR patch solves is:
>>> - stopping in ptrace_report_syscall.
>>> - Not having PT_TRACESYSGOOD set.
>>> - The tracee being killed with a fatal signal
>>         ^^^^^^
>>         tracer ?
>
> Both actually.
>
>>> - The tracee sending SIGTRAP to itself.
>>
>> Oh, but this is clear. But do we really care? If the tracer exits
>> unexpectedly, the tracee can have a lot more problems, I don't think
>> that this particular one is that important.
>
> I don't know of complaints, and if you haven't heard them either
> that that is a good indication that in practice we don't care.
>
> At a practical level I just don't want that silly case that sets
> TASK_TRACED to TASK_RUNNING without stopping at all in ptrace_stop to
> remain.  It just seems to make everything more complicated for no real
> reason anymore.  The deadlocks may_ptrace_stop was guarding against are
> gone.
>
> Plus the test is so racy we case can happen after we drop siglock
> before we schedule, or shortly after we have stopped so we really
> don't reliably catch the condition the code is trying to catch.
>
> I think the case I care most about is ptrace_signal, which pretty much
> requires the tracer to wait and clear exit_code before being terminated
> to cause problems.  We don't handle that at all today.
>
> So yeah.  I think the code handles so little at this point we can just
> remove the code and simplify things, if we actually care we can come
> back and implement JOBCTL_PTRACE_SIGNR or the like.

The original explanation for handling this is:

commit 66519f549ae516e7ff2f24a8a5134713411a4a58
Author: Roland McGrath <roland@...hat.com>
Date:   Tue Jan 4 05:38:15 2005 -0800

    [PATCH] fix ptracer death race yielding bogus BUG_ON
    
    There is a BUG_ON in ptrace_stop that hits if the thread is not ptraced.
    However, there is no synchronization between a thread deciding to do a
    ptrace stop and so going here, and its ptracer dying and so detaching from
    it and clearing its ->ptrace field.
    
    The RHEL3 2.4-based kernel has a backport of a slightly older version of
    the 2.6 signals code, which has a different but equivalent BUG_ON.  This
    actually bit users in practice (when the debugger dies), but was
    exceedingly difficult to reproduce in contrived circumstances.  We moved
    forward in RHEL3 just by removing the BUG_ON, and that fixed the real user
    problems even though I was never able to reproduce the scenario myself.
    So, to my knowledge this scenario has never actually been seen in practice
    under 2.6.  But it's plain to see from the code that it is indeed possible.
    
    This patch removes that BUG_ON, but also goes further and tries to handle
    this case more gracefully than simply avoiding the crash.  By removing the
    BUG_ON alone, it becomes possible for the real parent of a process to see
    spurious SIGCHLD notifications intended for the debugger that has just
    died, and have its child wind up stopped unexpectedly.  This patch avoids
    that possibility by detecting the case when we are about to do the ptrace
    stop but our ptracer has gone away, and simply eliding that ptrace stop
    altogether as if we hadn't been ptraced when we hit the interesting event
    (signal or ptrace_notify call for syscall tracing or something like that).
    
    Signed-off-by: Roland McGrath <roland@...hat.com>
    Signed-off-by: Andrew Morton <akpm@...l.org>
    Signed-off-by: Linus Torvalds <torvalds@...l.org>

And it was all about
	BUG_ON(!(current->ptrace & PT_PTRACED));
At the beginning of ptrace_stop.

Which seems like a bit of buggy overkill.

>
> I will chew on that a bit and see if I can find any reasons for keeping
> the code in ptrace_stop at all.

Still chewing.

Eric

Powered by blists - more mailing lists