From: Oleg Nesterov <oleg@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: rjw@rjwysocki.net, mingo@kernel.org, vincent.guittot@linaro.org,
dietmar.eggemann@arm.com, rostedt@goodmis.org, mgorman@suse.de,
ebiederm@xmission.com, bigeasy@linutronix.de,
Will Deacon <will@kernel.org>,
linux-kernel@vger.kernel.org, tj@kernel.org,
linux-pm@vger.kernel.org
Subject: Re: [PATCH 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT
Date: Fri, 15 Apr 2022 12:16:44 +0200 [thread overview]
Message-ID: <20220415101644.GA10421@redhat.com> (raw)
In-Reply-To: <YlikBjA3kL3XEQP5@hirez.programming.kicks-ass.net>
On 04/15, Peter Zijlstra wrote:
>
> On Thu, Apr 14, 2022 at 08:34:33PM +0200, Oleg Nesterov wrote:
>
> > If it can work, then 1/5 needs some changes, I think. In particular,
> > it should not introduce JOBCTL_TRACED_FROZEN until 5/5, and perhaps
>
> That TRACED_FROZEN was to distinguish the TASK_TRACED and __TASK_TRACED
> state, and isn't related to the freezer.
Lets forget about 3-5 which I didn't read carefully yet. So why do we
need TRACED_FROZEN?
From 1/5:
static inline void signal_wake_up(struct task_struct *t, bool resume)
{
+ lockdep_assert_held(&t->sighand->siglock);
+
+ if (resume && !(t->jobctl & JOBCTL_TRACED_FROZEN))
+ t->jobctl &= ~(JOBCTL_STOPPED | JOBCTL_TRACED);
+
signal_wake_up_state(t, resume ? TASK_WAKEKILL : 0);
}
+
static inline void ptrace_signal_wake_up(struct task_struct *t, bool resume)
{
+ lockdep_assert_held(&t->sighand->siglock);
+
+ if (resume)
+ t->jobctl &= ~JOBCTL_TRACED;
+
signal_wake_up_state(t, resume ? __TASK_TRACED : 0);
}
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);
}
?
> > /*
> > * We take the read lock around doing both checks to close a
> > * possible race where someone else attaches or detaches our
> > * natural child.
> > */
> > read_lock(&tasklist_lock);
> > traced = child->ptrace && child->parent == current;
> > read_unlock(&tasklist_lock);
> >
> > if (!traced)
> > return -ESRCH;
>
> The thing being, that if it is our ptrace child, it won't be going away
> since we're running this code and not ptrace_detach(). Right?
Yes. and nobody else can detach it.
Another tracer can't attach until child->ptrace is cleared, but this can
only happen if a) this child is killed and b) another thread does wait()
and reaps it; but after that attach() is obviously impossible.
But since this child can go away, the patch changes ptrace_freeze_traced()
to use lock_task_sighand().
> > for (;;) {
> > if (fatal_signal_pending(current))
> > return -EINTR;
>
> What if signal_wake_up(.resume=true) happens here? In that case we miss
> the fatal pending, and task state isn't changed yet so we'll happily go
> sleep.
No, it won't sleep, see the signal_pending_state() check in schedule().
> > set_current_state(TASK_KILLABLE);
And let me explain TASK_KILLABLE just in case... We could just use
TASK_UNINTERRUPTIBLE and avoid the signal_pending() check, but KILLABLE
looks "safer" to me. If the tracer hangs because of some bug, at least
it can be killed from userspace.
> > if (!(READ_ONCE(child->jobctl) & JOBCTL_TRACED)) {
>
> TRACED_XXX ?
oops ;)
> > - 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 ?
But! I forgot to make anothet change in this code. I do not think it should
rely on task_is_traced(). We are going to abuse task->__state, so I think
it should check task->__state == TASK_TRACED directly. Say,
if (READ_ONCE(task->__state) == TASK_TRACED && ...) {
WRITE_ONCE(task->__state, __TASK_TRACED);
WARN_ON_ONCE(!task_is_traced(task));
ret = true;
}
looks more clean to me. What do you think?
> > @@ -2307,13 +2313,14 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> > */
> > if (gstop_done)
> > do_notify_parent_cldstop(current, false, why);
> > + clear_traced_xxx();
> > + read_unlock(&tasklist_lock);
> >
> > - /* tasklist protects us from ptrace_freeze_traced() */
> > + /* JOBCTL_TRACED_XXX protects us from ptrace_freeze_traced() */
>
> But... TRACED_XXX has just been cleared ?!
Cough ;) OK, I'll move __set_current_state() back under tasklist.
And in this case we do not need wake_up(parent), so we can shift it from
clear_traced_xxx() into another branch.
OK, so far it seems that this patch needs a couple of simple fixes you
pointed out, but before I send V2:
- do you agree we can avoid JOBCTL_TRACED_FROZEN in 1-2 ?
- will you agree if I change ptrace_freeze_traced() to rely
on __state == TASK_TRACED rather than task_is_traced() ?
Thanks,
Oleg.
next prev parent reply other threads:[~2022-04-15 10:17 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-12 11:44 [PATCH 0/5] ptrace-vs-PREEMPT_RT and freezer rewrite Peter Zijlstra
2022-04-12 11:44 ` [PATCH 1/5] sched,signal,ptrace: Rework TASK_TRACED, TASK_STOPPED state Peter Zijlstra
2022-04-13 13:29 ` Oleg Nesterov
2022-04-13 16:47 ` Peter Zijlstra
2022-04-12 11:44 ` [PATCH 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT Peter Zijlstra
2022-04-13 13:24 ` Oleg Nesterov
2022-04-13 16:58 ` Peter Zijlstra
2022-04-13 18:57 ` Oleg Nesterov
2022-04-13 18:59 ` Oleg Nesterov
2022-04-13 19:20 ` Peter Zijlstra
2022-04-13 19:56 ` Peter Zijlstra
2022-04-14 11:54 ` Oleg Nesterov
2022-04-14 12:08 ` Oleg Nesterov
2022-04-14 18:34 ` Oleg Nesterov
2022-04-14 22:45 ` Peter Zijlstra
2022-04-15 10:16 ` Oleg Nesterov [this message]
2022-04-15 10:57 ` Oleg Nesterov
2022-04-15 12:01 ` Peter Zijlstra
2022-04-18 17:01 ` Oleg Nesterov
2022-04-18 17:19 ` Oleg Nesterov
2022-04-20 13:17 ` Peter Zijlstra
2022-04-20 18:03 ` Oleg Nesterov
2022-04-20 20:54 ` [RFC][PATCH] ptrace: Don't change __state Eric W. Biederman
2022-04-21 7:21 ` Peter Zijlstra
2022-04-21 10:26 ` Peter Zijlstra
2022-04-21 10:49 ` Oleg Nesterov
2022-04-21 11:50 ` Peter Zijlstra
2022-04-21 14:45 ` Eric W. Biederman
2022-04-21 9:46 ` Oleg Nesterov
2022-04-21 15:01 ` Eric W. Biederman
2022-04-27 0:51 ` [ptrace] [confidence: ] 7d3fafb751: BUG:sleeping_function_called_from_invalid_context_at_arch/x86/entry/common.c kernel test robot
2022-04-20 10:20 ` [PATCH 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT Peter Zijlstra
2022-04-20 11:35 ` Oleg Nesterov
2022-04-15 12:00 ` Peter Zijlstra
2022-04-15 12:56 ` Oleg Nesterov
2022-04-12 11:44 ` [PATCH 3/5] freezer: Have {,un}lock_system_sleep() save/restore flags Peter Zijlstra
2022-04-12 11:44 ` [PATCH 4/5] freezer,umh: Clean up freezer/initrd interaction Peter Zijlstra
2022-04-12 11:44 ` [PATCH 5/5] freezer,sched: Rewrite core freezer logic Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220415101644.GA10421@redhat.com \
--to=oleg@redhat.com \
--cc=bigeasy@linutronix.de \
--cc=dietmar.eggemann@arm.com \
--cc=ebiederm@xmission.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=rjw@rjwysocki.net \
--cc=rostedt@goodmis.org \
--cc=tj@kernel.org \
--cc=vincent.guittot@linaro.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).