Currently ptrace_check_attach() / ptrace_freeze_traced() rely on
specific scheduler behaviour to freeze the task. In specific, it
relies on there not being any blocking behaviour between:
set_special_state(TASK_TRACED);
...
schedule();
specifically it relies on being able to change p->__state between
those two points (to clear/set TASK_WAKEKILL) and relies on
wait_task_inactive() to ensure the task has hit that schedule().
However, PREEMPT_RT is breaking this by introducing sleeping
spinlocks. The consequence of sleeping spinlocks is that p->__state
can change (also see p->saved_state) and that a task can be inactive
(off the runqueue) and *NOT* be at the schedule() as expected.
That is, both the p->__state and wait_task_inactive() usage are
broken.
In order to avoid having to deal with p->saved_state, flip the
wait_task_inactive() and p->__state poking. That is, first wait for
the victim to be in schedule() and then poke p->__state, which is in a
known state by then.
The only problem with this is that it's possible to race with a
concurrent ptrace_detach()+pthread_attach() landing back in the same
situation as before. To deal with this, compare the task's nvcsw
count to make sure there is no scheduling between the initial
quiescence and the final task state poking.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
kernel/ptrace.c | 175 +++++++++++++++++++++++++++++++++++++++++-----------
kernel/sched/core.c | 5 -
2 files changed, 141 insertions(+), 39 deletions(-)
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -186,31 +186,13 @@ static bool looks_like_a_spurious_pid(st
}
/*
- * Ensure that nothing can wake it up, even SIGKILL
+ * Ptrace operation is complete, re-allow TASK_WAKEKILL.
*
- * A task is switched to this state while a ptrace operation is in progress;
- * such that the ptrace operation is uninterruptible.
+ * Unfreeze is easy, since ptrace_check_attach() guarantees the task is off the
+ * runqueue and __TASK_TRACED. If it's still __TASK_TRACED holding
+ * sighand->siglock serializes against ptrace_signal_wake_up() and we can
+ * simply put TASK_WAKEKILL back (or wake because there's a pending fatal).
*/
-static bool ptrace_freeze_traced(struct task_struct *task)
-{
- bool ret = false;
-
- /* Lockless, nobody but us can set this flag */
- if (task->jobctl & JOBCTL_LISTENING)
- return ret;
-
- 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;
- }
- spin_unlock_irq(&task->sighand->siglock);
-
- return ret;
-}
-
static void ptrace_unfreeze_traced(struct task_struct *task)
{
if (READ_ONCE(task->__state) != __TASK_TRACED)
@@ -234,6 +216,94 @@ static void ptrace_unfreeze_traced(struc
spin_unlock_irq(&task->sighand->siglock);
}
+/*
+ * In order to start a ptrace operation the victim task must be off the
+ * runqueue in state __TASK_TRACED.
+ */
+static int __ptrace_freeze_cond(struct task_struct *p)
+{
+ if (!task_is_traced(p))
+ return -ESRCH;
+
+ if (task_curr(p))
+ return -EINPROGRESS;
+
+ if (p->on_rq)
+ return -EWOULDBLOCK;
+
+ /*
+ * Due to PREEMPT_RT it is possible the task is blocked on a spinlock
+ * in state TASK_RTLOCK_WAIT, if so, gotta wait until that goes away.
+ */
+ if (!(READ_ONCE(p->__state) & __TASK_TRACED))
+ return -EWOULDBLOCK;
+
+ return 0;
+}
+
+/*
+ * Returns:
+ * 0: task is off runqueue in TASK_TRACED
+ * -ESRCH: not traced
+ * -EINPROGRESS: still running
+ * -EWOULDBLOCK: not running
+ */
+static int __ptrace_pre_freeze(struct task_struct *p, void *arg)
+{
+ int ret;
+
+ ret = __ptrace_freeze_cond(p);
+ if (ret)
+ return ret;
+
+ *(unsigned long *)arg = p->nvcsw;
+
+ return 0;
+}
+
+/*
+ * Returns:
+ * 0: task is off runqueue, now __TASK_TRACED
+ * -ESRCH: not traced, or scheduled since pre_freeze
+ * -GAIN: still running
+ * -EWOULDBLOCK: not running
+ */
+static int __ptrace_freeze(struct task_struct *p, void *arg)
+{
+ int ret;
+
+ ret = __ptrace_freeze_cond(p);
+ if (ret)
+ return ret;
+
+ /*
+ * Task scheduled between __ptrace_pre_freeze() and here, not our task
+ * anymore.
+ */
+ if (*(unsigned long *)arg != p->nvcsw)
+ return -ESRCH;
+
+ if (looks_like_a_spurious_pid(p))
+ return -ESRCH;
+
+ if (__fatal_signal_pending(p))
+ return -ESRCH;
+
+ /*
+ * we hold:
+ *
+ * - tasklist_lock (avoids ptrace_detach)
+ * - p->sighand->siglock (avoids ptrace_signal_wake_up)
+ * - p->pi_lock (avoids anything scheduler)
+ *
+ * task is absolutely frozen, now we can safely take out
+ * TASK_WAKEKILL.
+ */
+ p->jobctl |= JOBCTL_TRACED_FROZEN;
+ WRITE_ONCE(p->__state, __TASK_TRACED);
+ return 0;
+}
+
/**
* ptrace_check_attach - check whether ptracee is ready for ptrace operation
* @child: ptracee to check for
@@ -253,7 +323,36 @@ static void ptrace_unfreeze_traced(struc
*/
static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
{
- int ret = -ESRCH;
+ ktime_t to = TICK_NSEC;
+ unsigned long nvcsw;
+ int ret;
+
+ if (child == current)
+ return -ESRCH;
+
+ if (!ignore_state) for (;;) {
+ /*
+ * Ensure this child is a quiescent TASK_TRACED task.
+ */
+ ret = task_call_func(child, __ptrace_pre_freeze, &nvcsw);
+ switch (ret) {
+ case 0:
+ break;
+ case -ESRCH:
+ return ret;
+ case -EINPROGRESS:
+ while (task_curr(child))
+ cpu_relax();
+ continue;
+ case -EWOULDBLOCK:
+ /*
+ * XXX horrible, randomly wait 1 tick and try again.
+ */
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ schedule_hrtimeout(&to, HRTIMER_MODE_REL_HARD);
+ continue;
+ }
+ }
/*
* We take the read lock around doing both checks to close a
@@ -262,29 +361,35 @@ static int ptrace_check_attach(struct ta
* we are sure that this is our traced child and that can only
* be changed by us so it's not changing right after this.
*/
+ ret = -ESRCH;
read_lock(&tasklist_lock);
if (child->ptrace && child->parent == current) {
WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
+ if (ignore_state) {
+ ret = 0;
+ goto unlock;
+ }
+
+ if (child->jobctl & JOBCTL_LISTENING)
+ goto unlock;
+
/*
* child->sighand can't be NULL, release_task()
* does ptrace_unlink() before __exit_signal().
*/
- if (ignore_state || ptrace_freeze_traced(child))
- ret = 0;
- }
- read_unlock(&tasklist_lock);
-
- if (!ret && !ignore_state) {
- if (!wait_task_inactive(child, __TASK_TRACED)) {
+ spin_lock_irq(&child->sighand->siglock);
+ ret = task_call_func(child, __ptrace_freeze, &nvcsw);
+ if (ret) {
/*
- * This can only happen if may_ptrace_stop() fails and
- * ptrace_stop() changes ->state back to TASK_RUNNING,
- * so we should not worry about leaking __TASK_TRACED.
+ * If *anything* changed since __ptrace_pre_freeze()
+ * this fails.
*/
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
ret = -ESRCH;
}
+ spin_unlock_irq(&child->sighand->siglock);
}
+unlock:
+ read_unlock(&tasklist_lock);
return ret;
}
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6310,10 +6310,7 @@ static void __sched notrace __schedule(u
/*
* We must load prev->state once (task_struct::state is volatile), such
- * that:
- *
- * - we form a control dependency vs deactivate_task() below.
- * - ptrace_{,un}freeze_traced() can change ->state underneath us.
+ * that we form a control dependency vs deactivate_task() below.
*/
prev_state = READ_ONCE(prev->__state);
if (!(sched_mode & SM_MASK_PREEMPT) && prev_state) {
Hi Peter,
I like 1-2 but I need to read them (and other patches) again, a
couple of nits right now.
On 04/12, Peter Zijlstra wrote:
>
> +static int __ptrace_freeze_cond(struct task_struct *p)
> +{
> + if (!task_is_traced(p))
> + return -ESRCH;
if (!task_is_traced(p) || p->parent != current)
return -ESRCH;
we should not spin/sleep if it is traced by another task
> +static int __ptrace_freeze(struct task_struct *p, void *arg)
> +{
> + int ret;
> +
> + ret = __ptrace_freeze_cond(p);
> + if (ret)
> + return ret;
> +
> + /*
> + * Task scheduled between __ptrace_pre_freeze() and here, not our task
> + * anymore.
> + */
> + if (*(unsigned long *)arg != p->nvcsw)
> + return -ESRCH;
> +
> + if (looks_like_a_spurious_pid(p))
> + return -ESRCH;
Oh, I do not think __ptrace_freeze() should check for spurious pid...
looks_like_a_spurious_pid() should be called once in ptrace_check_attach()
before task_call_func(__ptrace_freeze).
Oleg.
On Wed, Apr 13, 2022 at 03:24:52PM +0200, Oleg Nesterov wrote:
> Hi Peter,
>
> I like 1-2 but I need to read them (and other patches) again, a
> couple of nits right now.
>
> On 04/12, Peter Zijlstra wrote:
> >
> > +static int __ptrace_freeze_cond(struct task_struct *p)
> > +{
> > + if (!task_is_traced(p))
> > + return -ESRCH;
>
> if (!task_is_traced(p) || p->parent != current)
> return -ESRCH;
>
> we should not spin/sleep if it is traced by another task
Yes, fair enough. And I suppose doing this test without holding siglock
is safe enough.
> > +static int __ptrace_freeze(struct task_struct *p, void *arg)
> > +{
> > + int ret;
> > +
> > + ret = __ptrace_freeze_cond(p);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * Task scheduled between __ptrace_pre_freeze() and here, not our task
> > + * anymore.
> > + */
> > + if (*(unsigned long *)arg != p->nvcsw)
> > + return -ESRCH;
> > +
> > + if (looks_like_a_spurious_pid(p))
> > + return -ESRCH;
>
> Oh, I do not think __ptrace_freeze() should check for spurious pid...
> looks_like_a_spurious_pid() should be called once in ptrace_check_attach()
> before task_call_func(__ptrace_freeze).
I can certainly do that, but since that needs be done with siglock held,
and the __ptrace_freeze call is a one-time affair, I didn't really see
the point in making the code more complicated.
Something like so then?
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -222,7 +222,7 @@ static void ptrace_unfreeze_traced(struc
*/
static int __ptrace_freeze_cond(struct task_struct *p)
{
- if (!task_is_traced(p))
+ if (!task_is_traced(p) || p->parent != current)
return -ESRCH;
if (task_curr(p))
@@ -283,9 +283,6 @@ static int __ptrace_freeze(struct task_s
if (*(unsigned long *)arg != p->nvcsw)
return -ESRCH;
- if (looks_like_a_spurious_pid(p))
- return -ESRCH;
-
if (__fatal_signal_pending(p))
return -ESRCH;
@@ -378,6 +375,9 @@ static int ptrace_check_attach(struct ta
* does ptrace_unlink() before __exit_signal().
*/
spin_lock_irq(&child->sighand->siglock);
+ if (looks_like_a_spurious_pid(child))
+ goto unlock_sig;
+
ret = task_call_func(child, __ptrace_freeze, &nvcsw);
if (ret) {
/*
@@ -386,6 +386,7 @@ static int ptrace_check_attach(struct ta
*/
ret = -ESRCH;
}
+unlock_sig:
spin_unlock_irq(&child->sighand->siglock);
}
unlock:
On Wed, Apr 13, 2022 at 08:59:10PM +0200, Oleg Nesterov wrote:
> + // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> + // wrong, needs siglock
> + current->jobctl &= ~JOBCTL_TRACED_XXX;
> + wake_up_bit(¤t->jobctl, ~JOBCTL_TRACED_XXX_BIT);
__wake_up_common_lock()
spin_lock_irqsave()
current_save_and_set_rtlock_wait_state();
> + if (wait_on_bit(&task->jobctl, JOBCTL_TRACED_XXX_BIT, TASK_KILLABLE))
> + return -EINTR;
> + // now that the tracee cleared JOBCTL_TRACED_XXX_BIT
> + // wait_task_inactive() should succeed or fail "really soon".
> + if (!wait_task_inactive(child, __TASK_TRACED))
> + return ret;
*whoopsie* ?
> preempt_enable_no_resched();
> freezable_schedule();
> cgroup_leave_frozen(true);
On Wed, Apr 13, 2022 at 09:20:53PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 08:59:10PM +0200, Oleg Nesterov wrote:
>
>
> > + // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> > + // wrong, needs siglock
> > + current->jobctl &= ~JOBCTL_TRACED_XXX;
> > + wake_up_bit(¤t->jobctl, ~JOBCTL_TRACED_XXX_BIT);
> __wake_up_common_lock()
> spin_lock_irqsave()
> current_save_and_set_rtlock_wait_state();
>
> > + if (wait_on_bit(&task->jobctl, JOBCTL_TRACED_XXX_BIT, TASK_KILLABLE))
> > + return -EINTR;
> > + // now that the tracee cleared JOBCTL_TRACED_XXX_BIT
> > + // wait_task_inactive() should succeed or fail "really soon".
> > + if (!wait_task_inactive(child, __TASK_TRACED))
> > + return ret;
>
>
> *whoopsie* ?
>
> > preempt_enable_no_resched();
> > freezable_schedule();
> > cgroup_leave_frozen(true);
Something that might work; since there's only the one ptracer, is to
instead do something like:
current->jobctl &= ~JOBCTL_TRACED_XXX; // siglock
if (current->ptrace)
wake_up_process(current->parent);
preempt_enable_no_resched();
schedule();
vs
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
if (!(p->jobctl & JOBCTL_TRACED_XXX))
break;
schedule();
}
__set_current_state(TASK_RUNNING);
if (!wait_task_inactive(...))
ptrace_detach() needs some additional magic as well I think, but this
might just work.
On 04/13, Oleg Nesterov wrote:
>
> On 04/13, Oleg Nesterov wrote:
> >
> > I like 1-2 but I need to read them (and other patches) again, a
> > couple of nits right now.
>
> Sorry, didn't have time to do this today, and now I am already sleeping.
>
> But... on a second thought, it seems there is a better solution. If nothing
> else it is simpler and doesn't duplicate the wait_task_inactive() logic.
>
> How about the patch below instead? On top of 1/5.
>
> Yes,yes, incomplete. in particular see the "!!!!!!!!!" comments. Just to
> explain the idea.
Cough. forget to attach the patch, sorry for noise.
Oleg.
---
diff --git a/include/linux/sched/jobctl.h b/include/linux/sched/jobctl.h
index ec8b312f7506..1b5a57048e13 100644
--- a/include/linux/sched/jobctl.h
+++ b/include/linux/sched/jobctl.h
@@ -22,7 +22,8 @@ struct task_struct;
#define JOBCTL_STOPPED_BIT 24
#define JOBCTL_TRACED_BIT 25
-#define JOBCTL_TRACED_FROZEN_BIT 26
+#define JOBCTL_TRACED_XXX_BIT 25
+#define JOBCTL_TRACED_FROZEN_BIT 27
#define JOBCTL_STOP_DEQUEUED (1UL << JOBCTL_STOP_DEQUEUED_BIT)
#define JOBCTL_STOP_PENDING (1UL << JOBCTL_STOP_PENDING_BIT)
@@ -35,6 +36,7 @@ struct task_struct;
#define JOBCTL_STOPPED (1UL << JOBCTL_STOPPED_BIT)
#define JOBCTL_TRACED (1UL << JOBCTL_TRACED_BIT)
+#define JOBCTL_TRACED_XXX (1UL << JOBCTL_TRACED_XXX_BIT)
#define JOBCTL_TRACED_FROZEN (1UL << JOBCTL_TRACED_FROZEN_BIT)
#define JOBCTL_TRAP_MASK (JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 626f96d275c7..86b5226e6ba2 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -255,6 +255,19 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
{
int ret = -ESRCH;
+ if (!(child->ptrace && child->parent == current))
+ return ret;
+
+ if (ignore_state)
+ return 0;
+
+ if (wait_on_bit(&task->jobctl, JOBCTL_TRACED_XXX_BIT, TASK_KILLABLE))
+ return -EINTR;
+ // now that the tracee cleared JOBCTL_TRACED_XXX_BIT
+ // wait_task_inactive() should succeed or fail "really soon".
+ if (!wait_task_inactive(child, __TASK_TRACED))
+ return ret;
+
/*
* We take the read lock around doing both checks to close a
* possible race where someone else was tracing our child and
@@ -269,23 +282,11 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
* child->sighand can't be NULL, release_task()
* does ptrace_unlink() before __exit_signal().
*/
- if (ignore_state || ptrace_freeze_traced(child))
+ if (ptrace_freeze_traced(child))
ret = 0;
}
read_unlock(&tasklist_lock);
- if (!ret && !ignore_state) {
- if (!wait_task_inactive(child, __TASK_TRACED)) {
- /*
- * This can only happen if may_ptrace_stop() fails and
- * ptrace_stop() changes ->state back to TASK_RUNNING,
- * so we should not worry about leaking __TASK_TRACED.
- */
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
- ret = -ESRCH;
- }
- }
-
return ret;
}
diff --git a/kernel/signal.c b/kernel/signal.c
index 0aea3f0a8002..5ca6235e5231 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2220,7 +2220,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
* schedule() will not sleep if there is a pending signal that
* can awaken the task.
*/
- current->jobctl |= JOBCTL_TRACED;
+ current->jobctl |= (JOBCTL_TRACED | JOBCTL_TRACED_XXX);
set_special_state(TASK_TRACED);
/*
@@ -2291,6 +2291,10 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
preempt_disable();
read_unlock(&tasklist_lock);
cgroup_enter_frozen();
+ // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+ // wrong, needs siglock
+ current->jobctl &= ~JOBCTL_TRACED_XXX;
+ wake_up_bit(¤t->jobctl, ~JOBCTL_TRACED_XXX_BIT);
preempt_enable_no_resched();
freezable_schedule();
cgroup_leave_frozen(true);
@@ -2308,6 +2312,8 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
if (gstop_done)
do_notify_parent_cldstop(current, false, why);
+ // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+ // need to clear ~JOBCTL_TRACED_XXX
/* tasklist protects us from ptrace_freeze_traced() */
__set_current_state(TASK_RUNNING);
read_code = false;
On 04/13, Oleg Nesterov wrote:
>
> I like 1-2 but I need to read them (and other patches) again, a
> couple of nits right now.
Sorry, didn't have time to do this today, and now I am already sleeping.
But... on a second thought, it seems there is a better solution. If nothing
else it is simpler and doesn't duplicate the wait_task_inactive() logic.
How about the patch below instead? On top of 1/5.
Yes,yes, incomplete. in particular see the "!!!!!!!!!" comments. Just to
explain the idea.
Oleg.
On 04/13, Peter Zijlstra wrote:
>
> On Wed, Apr 13, 2022 at 09:20:53PM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 13, 2022 at 08:59:10PM +0200, Oleg Nesterov wrote:
> >
> >
> > > + // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> > > + // wrong, needs siglock
> > > + current->jobctl &= ~JOBCTL_TRACED_XXX;
> > > + wake_up_bit(¤t->jobctl, ~JOBCTL_TRACED_XXX_BIT);
> > __wake_up_common_lock()
> > spin_lock_irqsave()
> > current_save_and_set_rtlock_wait_state();
OOPS, thanks.
> Something that might work; since there's only the one ptracer, is to
> instead do something like:
>
> current->jobctl &= ~JOBCTL_TRACED_XXX; // siglock
> if (current->ptrace)
> wake_up_process(current->parent);
> preempt_enable_no_resched();
> schedule();
>
>
> vs
>
> for (;;) {
> set_current_state(TASK_UNINTERRUPTIBLE);
> if (!(p->jobctl & JOBCTL_TRACED_XXX))
> break;
> schedule();
Yes, thanks... this is racy, see below, but probably fixeable.
> ptrace_detach() needs some additional magic as well I think, but this
> might just work.
I don't think so, JOBCTL_TRACED_XXX must be always cleared in ptrace_stop()
and ptrace_detach() implies ptrace_check_attach().
Lets forget about the proble above for the moment. There is another problem
with my patch,
if (!(child->ptrace && child->parent == current))
return ret;
this check is racy without tasklist, we can race with another task attaching
to our natural child (so that child->parent == current), ptrace_attach() sets
task->ptrace = flags first and changes child->parent after that.
In this case:
if (ignore_state)
return 0;
this is just wrong,
if (wait_on_bit(&task->jobctl, JOBCTL_TRACED_XXX_BIT, TASK_KILLABLE))
return -EINTR;
this is fine,
if (!wait_task_inactive(child, __TASK_TRACED))
this not right too. wait_task_inactive() can loop "forever" doing schedule_hrtimeout()
if the actual debugger stops/resumes the tracee continuously. This is pure theoretical,
but still.
And this also means that the code above needs some changes too, we can rely on
wake_up_process(current->parent).
OK, let me think about it. Thanks!
Oleg.
On 04/15, Oleg Nesterov wrote:
>
> 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() ?
>
Forgot to say, I think 1/5 needs some changes in any case...
ptrace_resume() does wake_up_state(child, __TASK_TRACED) but doesn't
clear JOBCTL_TRACED. The "else" branch in ptrace_stop() leaks this flag
too. Perhaps I missed something, I'll reread 1/5 again, but the main
question to me is whether 1-2 actually need the JOBCTL_TRACED_FROZEN flag.
Oleg.
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.
this doesn't really matter, just for completeness:
On 04/14, Oleg Nesterov wrote:
>
> if (wait_on_bit(&task->jobctl, JOBCTL_TRACED_XXX_BIT, TASK_KILLABLE))
> return -EINTR;
>
> this is fine,
No, this is wrong too. wake_up_bit() does exclusive wakeup.
Oleg.
On Fri, Apr 15, 2022 at 12:57:56PM +0200, Oleg Nesterov wrote:
> On 04/15, Oleg Nesterov wrote:
> >
> > 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() ?
> >
>
> Forgot to say, I think 1/5 needs some changes in any case...
>
> ptrace_resume() does wake_up_state(child, __TASK_TRACED) but doesn't
> clear JOBCTL_TRACED. The "else" branch in ptrace_stop() leaks this flag
> too.
Urgh, yes, I seemed to have missed that one :-(
On Fri, Apr 15, 2022 at 12:16:44PM +0200, Oleg Nesterov wrote:
> 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?
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).
> 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);
> }
>
> ?
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.
If not broken, then it needs at least a comment explaining why not etc..
I'm sure to not remember many of these details.
Also, signal_wake_up_state() really can do with that
lockdep_assert_held() as well ;-)
> > > /*
> > > * 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().
Right.
> > > 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().
Urgh, forgot about that one ;-)
> > > 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.
Agreed.
>
> > > 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 ?
Yes, but I'm starting to feel a little paranoid here. Better safe than
sorry etc..
> 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?
Agreed on this.
> > > @@ -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 ?
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?
> - will you agree if I change ptrace_freeze_traced() to rely
> on __state == TASK_TRACED rather than task_is_traced() ?
Yes.
On Thu, Apr 14, 2022 at 08:34:33PM +0200, Oleg Nesterov wrote:
> On 04/14, Oleg Nesterov wrote:
> >
> > OK, let me think about it. Thanks!
>
> So. what do you think about the patch below?
Might just work, will have to look at it again though ;-) Comments
below.
> 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.
> we can avoid this flag altogether...
>
> This is how ptrace_check_attach() looks with the patch applied:
>
> static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
> {
> int traced;
> /*
> * 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?
I failed to realize this earlier and I think that's part of why my
solution is somewhat over complicated.
> WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
> if (ignore_state)
> return 0;
>
> 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.
> set_current_state(TASK_KILLABLE);
> if (!(READ_ONCE(child->jobctl) & JOBCTL_TRACED)) {
TRACED_XXX ?
> __set_current_state(TASK_RUNNING);
> break;
> }
> schedule();
> }
That is, shouldn't we write this like:
for (;;) {
set_current_state(TASK_KILLABLE);
if (fatal_signal_pending(current)) {
ret = -EINTR;
break;
}
if (!(READ_ONCE(current->jobctl) & JOBCTL_TRACED_XXX)) {
ret = 0;
break;
}
schedule();
}
__set_current_state(TASK_RUNNING);
if (ret)
return ret;
> if (!wait_task_inactive(child, TASK_TRACED) ||
> !ptrace_freeze_traced(child))
> return -ESRCH;
>
> return 0;
> }
>
> Oleg.
> ---
>
> diff --git a/include/linux/sched/jobctl.h b/include/linux/sched/jobctl.h
> index ec8b312f7506..1b5a57048e13 100644
> --- a/include/linux/sched/jobctl.h
> +++ b/include/linux/sched/jobctl.h
> @@ -22,7 +22,8 @@ struct task_struct;
>
> #define JOBCTL_STOPPED_BIT 24
> #define JOBCTL_TRACED_BIT 25
> -#define JOBCTL_TRACED_FROZEN_BIT 26
> +#define JOBCTL_TRACED_XXX_BIT 25
> +#define JOBCTL_TRACED_FROZEN_BIT 27
>
> #define JOBCTL_STOP_DEQUEUED (1UL << JOBCTL_STOP_DEQUEUED_BIT)
> #define JOBCTL_STOP_PENDING (1UL << JOBCTL_STOP_PENDING_BIT)
> @@ -35,6 +36,7 @@ struct task_struct;
>
> #define JOBCTL_STOPPED (1UL << JOBCTL_STOPPED_BIT)
> #define JOBCTL_TRACED (1UL << JOBCTL_TRACED_BIT)
> +#define JOBCTL_TRACED_XXX (1UL << JOBCTL_TRACED_XXX_BIT)
> #define JOBCTL_TRACED_FROZEN (1UL << JOBCTL_TRACED_FROZEN_BIT)
>
> #define JOBCTL_TRAP_MASK (JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 626f96d275c7..5a03ae5cb0c0 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -193,20 +193,22 @@ static bool looks_like_a_spurious_pid(struct task_struct *task)
> */
> static bool ptrace_freeze_traced(struct task_struct *task)
> {
> + unsigned long flags;
> bool ret = false;
>
> /* Lockless, nobody but us can set this flag */
> if (task->jobctl & JOBCTL_LISTENING)
> return ret;
> + if (!lock_task_sighand(task, &flags))
> + return ret;
>
> - 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.
> - spin_unlock_irq(&task->sighand->siglock);
> + unlock_task_sighand(task, &flags);
>
> return ret;
> }
> @@ -253,40 +255,39 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
> */
> static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
> {
> - int ret = -ESRCH;
> -
> + int traced;
> /*
> * We take the read lock around doing both checks to close a
> - * possible race where someone else was tracing our child and
> - * detached between these two checks. After this locked check,
> - * we are sure that this is our traced child and that can only
> - * be changed by us so it's not changing right after this.
> + * possible race where someone else attaches or detaches our
> + * natural child.
> */
> read_lock(&tasklist_lock);
> - if (child->ptrace && child->parent == current) {
> - WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
> - /*
> - * child->sighand can't be NULL, release_task()
> - * does ptrace_unlink() before __exit_signal().
> - */
> - if (ignore_state || ptrace_freeze_traced(child))
> - ret = 0;
> - }
> + traced = child->ptrace && child->parent == current;
> read_unlock(&tasklist_lock);
>
> - if (!ret && !ignore_state) {
> - if (!wait_task_inactive(child, __TASK_TRACED)) {
> - /*
> - * This can only happen if may_ptrace_stop() fails and
> - * ptrace_stop() changes ->state back to TASK_RUNNING,
> - * so we should not worry about leaking __TASK_TRACED.
> - */
> - WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
> - ret = -ESRCH;
> + if (!traced)
> + return -ESRCH;
> +
> + WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
> + if (ignore_state)
> + return 0;
> +
> + for (;;) {
> + if (fatal_signal_pending(current))
> + return -EINTR;
> + set_current_state(TASK_KILLABLE);
> + if (!(READ_ONCE(child->jobctl) & JOBCTL_TRACED)) {
> + __set_current_state(TASK_RUNNING);
> + break;
> }
> + schedule();
> }
>
> - return ret;
> + if (!wait_task_inactive(child, TASK_TRACED) ||
> + !ptrace_freeze_traced(child))
> + return -ESRCH;
> +
> + return 0;
> }
>
> static bool ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 0aea3f0a8002..684f0a0e9c71 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2182,6 +2182,14 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
> spin_unlock_irqrestore(&sighand->siglock, flags);
> }
>
> +static void clear_traced_xxx(void)
> +{
> + spin_lock_irq(¤t->sighand->siglock);
> + current->jobctl &= ~JOBCTL_TRACED_XXX;
> + spin_unlock_irq(¤t->sighand->siglock);
> + wake_up_state(current->parent, TASK_KILLABLE);
> +}
> +
> /*
> * This must be called with current->sighand->siglock held.
> *
> @@ -2220,7 +2228,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> * schedule() will not sleep if there is a pending signal that
> * can awaken the task.
> */
> - current->jobctl |= JOBCTL_TRACED;
> + current->jobctl |= JOBCTL_TRACED | JOBCTL_TRACED_XXX;
> set_special_state(TASK_TRACED);
>
> /*
> @@ -2282,6 +2290,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> if (gstop_done && ptrace_reparented(current))
> do_notify_parent_cldstop(current, false, why);
>
> + clear_traced_xxx();
> /*
> * Don't want to allow preemption here, because
> * sys_ptrace() needs this task to be inactive.
> @@ -2296,9 +2305,6 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> cgroup_leave_frozen(true);
> } else {
> /*
> - * By the time we got the lock, our tracer went away.
> - * Don't drop the lock yet, another tracer may come.
> - *
> * If @gstop_done, the ptracer went away between group stop
> * completion and here. During detach, it would have set
> * JOBCTL_STOP_PENDING on us and we'll re-enter
> @@ -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 ?!
> __set_current_state(TASK_RUNNING);
> read_code = false;
> if (clear_code)
> exit_code = 0;
> - read_unlock(&tasklist_lock);
> }
>
> /*
>
On 04/14, Oleg Nesterov wrote:
>
> OK, let me think about it. Thanks!
So. what do you think about the patch below?
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
we can avoid this flag altogether...
This is how ptrace_check_attach() looks with the patch applied:
static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
{
int traced;
/*
* 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;
WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
if (ignore_state)
return 0;
for (;;) {
if (fatal_signal_pending(current))
return -EINTR;
set_current_state(TASK_KILLABLE);
if (!(READ_ONCE(child->jobctl) & JOBCTL_TRACED)) {
__set_current_state(TASK_RUNNING);
break;
}
schedule();
}
if (!wait_task_inactive(child, TASK_TRACED) ||
!ptrace_freeze_traced(child))
return -ESRCH;
return 0;
}
Oleg.
---
diff --git a/include/linux/sched/jobctl.h b/include/linux/sched/jobctl.h
index ec8b312f7506..1b5a57048e13 100644
--- a/include/linux/sched/jobctl.h
+++ b/include/linux/sched/jobctl.h
@@ -22,7 +22,8 @@ struct task_struct;
#define JOBCTL_STOPPED_BIT 24
#define JOBCTL_TRACED_BIT 25
-#define JOBCTL_TRACED_FROZEN_BIT 26
+#define JOBCTL_TRACED_XXX_BIT 25
+#define JOBCTL_TRACED_FROZEN_BIT 27
#define JOBCTL_STOP_DEQUEUED (1UL << JOBCTL_STOP_DEQUEUED_BIT)
#define JOBCTL_STOP_PENDING (1UL << JOBCTL_STOP_PENDING_BIT)
@@ -35,6 +36,7 @@ struct task_struct;
#define JOBCTL_STOPPED (1UL << JOBCTL_STOPPED_BIT)
#define JOBCTL_TRACED (1UL << JOBCTL_TRACED_BIT)
+#define JOBCTL_TRACED_XXX (1UL << JOBCTL_TRACED_XXX_BIT)
#define JOBCTL_TRACED_FROZEN (1UL << JOBCTL_TRACED_FROZEN_BIT)
#define JOBCTL_TRAP_MASK (JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 626f96d275c7..5a03ae5cb0c0 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -193,20 +193,22 @@ static bool looks_like_a_spurious_pid(struct task_struct *task)
*/
static bool ptrace_freeze_traced(struct task_struct *task)
{
+ unsigned long flags;
bool ret = false;
/* Lockless, nobody but us can set this flag */
if (task->jobctl & JOBCTL_LISTENING)
return ret;
+ if (!lock_task_sighand(task, &flags))
+ return ret;
- 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;
}
- spin_unlock_irq(&task->sighand->siglock);
+ unlock_task_sighand(task, &flags);
return ret;
}
@@ -253,40 +255,39 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
*/
static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
{
- int ret = -ESRCH;
-
+ int traced;
/*
* We take the read lock around doing both checks to close a
- * possible race where someone else was tracing our child and
- * detached between these two checks. After this locked check,
- * we are sure that this is our traced child and that can only
- * be changed by us so it's not changing right after this.
+ * possible race where someone else attaches or detaches our
+ * natural child.
*/
read_lock(&tasklist_lock);
- if (child->ptrace && child->parent == current) {
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
- /*
- * child->sighand can't be NULL, release_task()
- * does ptrace_unlink() before __exit_signal().
- */
- if (ignore_state || ptrace_freeze_traced(child))
- ret = 0;
- }
+ traced = child->ptrace && child->parent == current;
read_unlock(&tasklist_lock);
- if (!ret && !ignore_state) {
- if (!wait_task_inactive(child, __TASK_TRACED)) {
- /*
- * This can only happen if may_ptrace_stop() fails and
- * ptrace_stop() changes ->state back to TASK_RUNNING,
- * so we should not worry about leaking __TASK_TRACED.
- */
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
- ret = -ESRCH;
+ if (!traced)
+ return -ESRCH;
+
+ WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
+ if (ignore_state)
+ return 0;
+
+ for (;;) {
+ if (fatal_signal_pending(current))
+ return -EINTR;
+ set_current_state(TASK_KILLABLE);
+ if (!(READ_ONCE(child->jobctl) & JOBCTL_TRACED)) {
+ __set_current_state(TASK_RUNNING);
+ break;
}
+ schedule();
}
- return ret;
+ if (!wait_task_inactive(child, TASK_TRACED) ||
+ !ptrace_freeze_traced(child))
+ return -ESRCH;
+
+ return 0;
}
static bool ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
diff --git a/kernel/signal.c b/kernel/signal.c
index 0aea3f0a8002..684f0a0e9c71 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2182,6 +2182,14 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
spin_unlock_irqrestore(&sighand->siglock, flags);
}
+static void clear_traced_xxx(void)
+{
+ spin_lock_irq(¤t->sighand->siglock);
+ current->jobctl &= ~JOBCTL_TRACED_XXX;
+ spin_unlock_irq(¤t->sighand->siglock);
+ wake_up_state(current->parent, TASK_KILLABLE);
+}
+
/*
* This must be called with current->sighand->siglock held.
*
@@ -2220,7 +2228,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
* schedule() will not sleep if there is a pending signal that
* can awaken the task.
*/
- current->jobctl |= JOBCTL_TRACED;
+ current->jobctl |= JOBCTL_TRACED | JOBCTL_TRACED_XXX;
set_special_state(TASK_TRACED);
/*
@@ -2282,6 +2290,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
if (gstop_done && ptrace_reparented(current))
do_notify_parent_cldstop(current, false, why);
+ clear_traced_xxx();
/*
* Don't want to allow preemption here, because
* sys_ptrace() needs this task to be inactive.
@@ -2296,9 +2305,6 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
cgroup_leave_frozen(true);
} else {
/*
- * By the time we got the lock, our tracer went away.
- * Don't drop the lock yet, another tracer may come.
- *
* If @gstop_done, the ptracer went away between group stop
* completion and here. During detach, it would have set
* JOBCTL_STOP_PENDING on us and we'll re-enter
@@ -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() */
__set_current_state(TASK_RUNNING);
read_code = false;
if (clear_code)
exit_code = 0;
- read_unlock(&tasklist_lock);
}
/*
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.
On 04/15, Peter Zijlstra wrote:
>
> On Fri, Apr 15, 2022 at 12:57:56PM +0200, Oleg Nesterov wrote:
> > On 04/15, Oleg Nesterov wrote:
> > >
> > > 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() ?
> > >
> >
> > Forgot to say, I think 1/5 needs some changes in any case...
> >
> > ptrace_resume() does wake_up_state(child, __TASK_TRACED) but doesn't
> > clear JOBCTL_TRACED. The "else" branch in ptrace_stop() leaks this flag
> > too.
>
> Urgh, yes, I seemed to have missed that one :-(
OK, I'll wait for updated version and send V3 on top of it.
I think the fixes are simple. ptrace_resume() should simply remove the
minor "need_siglock" optimization and clear JOBCTL_TRACED. As for the
"else" branch in ptrace_stop(), perhaps 1/5 can introduce the trivial
static void clear_traced_jobctl_flags(unsigned long flags)
{
spin_lock_irq(¤t->sighand->siglock);
current->jobctl &= ~flags;
spin_unlock_irq(¤t->sighand->siglock);
}
which can be reused by 2/5 to clear JOBCTL_TRACED_XXX. And, Peter, feel
free to join 1/5 and this patch if you think this makes any sense.
Meanwhile see V2 on top of the current version.
Oleg.
diff --git a/include/linux/sched/jobctl.h b/include/linux/sched/jobctl.h
index ec8b312f7506..1b5a57048e13 100644
--- a/include/linux/sched/jobctl.h
+++ b/include/linux/sched/jobctl.h
@@ -22,7 +22,8 @@ struct task_struct;
#define JOBCTL_STOPPED_BIT 24
#define JOBCTL_TRACED_BIT 25
-#define JOBCTL_TRACED_FROZEN_BIT 26
+#define JOBCTL_TRACED_XXX_BIT 25
+#define JOBCTL_TRACED_FROZEN_BIT 27
#define JOBCTL_STOP_DEQUEUED (1UL << JOBCTL_STOP_DEQUEUED_BIT)
#define JOBCTL_STOP_PENDING (1UL << JOBCTL_STOP_PENDING_BIT)
@@ -35,6 +36,7 @@ struct task_struct;
#define JOBCTL_STOPPED (1UL << JOBCTL_STOPPED_BIT)
#define JOBCTL_TRACED (1UL << JOBCTL_TRACED_BIT)
+#define JOBCTL_TRACED_XXX (1UL << JOBCTL_TRACED_XXX_BIT)
#define JOBCTL_TRACED_FROZEN (1UL << JOBCTL_TRACED_FROZEN_BIT)
#define JOBCTL_TRAP_MASK (JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 626f96d275c7..ec104bae4e21 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -193,20 +193,24 @@ static bool looks_like_a_spurious_pid(struct task_struct *task)
*/
static bool ptrace_freeze_traced(struct task_struct *task)
{
+ unsigned long flags;
bool ret = false;
/* Lockless, nobody but us can set this flag */
if (task->jobctl & JOBCTL_LISTENING)
return ret;
+ if (!lock_task_sighand(task, &flags))
+ return ret;
- spin_lock_irq(&task->sighand->siglock);
- if (task_is_traced(task) && !looks_like_a_spurious_pid(task) &&
+ if (READ_ONCE(task->__state) == TASK_TRACED &&
+ !looks_like_a_spurious_pid(task) &&
!__fatal_signal_pending(task)) {
task->jobctl |= JOBCTL_TRACED_FROZEN;
WRITE_ONCE(task->__state, __TASK_TRACED);
+ WARN_ON_ONCE(!task_is_traced(task));
ret = true;
}
- spin_unlock_irq(&task->sighand->siglock);
+ unlock_task_sighand(task, &flags);
return ret;
}
@@ -253,40 +257,42 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
*/
static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
{
- int ret = -ESRCH;
-
+ int traced;
/*
* We take the read lock around doing both checks to close a
- * possible race where someone else was tracing our child and
- * detached between these two checks. After this locked check,
- * we are sure that this is our traced child and that can only
- * be changed by us so it's not changing right after this.
+ * possible race where someone else attaches or detaches our
+ * natural child.
*/
read_lock(&tasklist_lock);
- if (child->ptrace && child->parent == current) {
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
- /*
- * child->sighand can't be NULL, release_task()
- * does ptrace_unlink() before __exit_signal().
- */
- if (ignore_state || ptrace_freeze_traced(child))
- ret = 0;
- }
+ traced = child->ptrace && child->parent == current;
read_unlock(&tasklist_lock);
- if (!ret && !ignore_state) {
- if (!wait_task_inactive(child, __TASK_TRACED)) {
- /*
- * This can only happen if may_ptrace_stop() fails and
- * ptrace_stop() changes ->state back to TASK_RUNNING,
- * so we should not worry about leaking __TASK_TRACED.
- */
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
- ret = -ESRCH;
+ if (!traced)
+ return -ESRCH;
+
+ WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
+ if (ignore_state)
+ return 0;
+
+ if (!task_is_traced(child))
+ return -ESRCH;
+
+ for (;;) {
+ if (fatal_signal_pending(current))
+ return -EINTR;
+ set_current_state(TASK_KILLABLE);
+ if (!(READ_ONCE(child->jobctl) & JOBCTL_TRACED_XXX)) {
+ __set_current_state(TASK_RUNNING);
+ break;
}
+ schedule();
}
- return ret;
+ if (!wait_task_inactive(child, TASK_TRACED) ||
+ !ptrace_freeze_traced(child))
+ return -ESRCH;
+
+ return 0;
}
static bool ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
diff --git a/kernel/signal.c b/kernel/signal.c
index 0aea3f0a8002..c7a89904cc4a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2182,6 +2182,13 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
spin_unlock_irqrestore(&sighand->siglock, flags);
}
+static void clear_traced_xxx(void)
+{
+ spin_lock_irq(¤t->sighand->siglock);
+ current->jobctl &= ~JOBCTL_TRACED_XXX;
+ spin_unlock_irq(¤t->sighand->siglock);
+}
+
/*
* This must be called with current->sighand->siglock held.
*
@@ -2220,7 +2227,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
* schedule() will not sleep if there is a pending signal that
* can awaken the task.
*/
- current->jobctl |= JOBCTL_TRACED;
+ current->jobctl |= JOBCTL_TRACED | JOBCTL_TRACED_XXX;
set_special_state(TASK_TRACED);
/*
@@ -2282,6 +2289,8 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
if (gstop_done && ptrace_reparented(current))
do_notify_parent_cldstop(current, false, why);
+ clear_traced_xxx();
+ wake_up_state(current->parent, TASK_KILLABLE);
/*
* Don't want to allow preemption here, because
* sys_ptrace() needs this task to be inactive.
@@ -2297,8 +2306,12 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
} else {
/*
* By the time we got the lock, our tracer went away.
- * Don't drop the lock yet, another tracer may come.
- *
+ * Don't drop the lock yet, another tracer may come,
+ * tasklist protects us from ptrace_freeze_traced().
+ */
+ __set_current_state(TASK_RUNNING);
+ clear_traced_xxx();
+ /*
* If @gstop_done, the ptracer went away between group stop
* completion and here. During detach, it would have set
* JOBCTL_STOP_PENDING on us and we'll re-enter
@@ -2307,13 +2320,11 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
*/
if (gstop_done)
do_notify_parent_cldstop(current, false, why);
+ read_unlock(&tasklist_lock);
- /* tasklist protects us from ptrace_freeze_traced() */
- __set_current_state(TASK_RUNNING);
read_code = false;
if (clear_code)
exit_code = 0;
- read_unlock(&tasklist_lock);
}
/*
On 04/20, Peter Zijlstra wrote:
>
> Does something like:
>
> #define JOBCTL_TRACED_BIT 25
> #define JOBCTL_TRACED_QUIESCE_BIT 26
>
> work?
Agreed! ;)
> > } else {
> > /*
> > * By the time we got the lock, our tracer went away.
> > - * Don't drop the lock yet, another tracer may come.
> > - *
> > + * Don't drop the lock yet, another tracer may come,
> > + * tasklist protects us from ptrace_freeze_traced().
> > + */
> > + __set_current_state(TASK_RUNNING);
> > + clear_traced_xxx();
> > + /*
> > * If @gstop_done, the ptracer went away between group stop
> > * completion and here. During detach, it would have set
> > * JOBCTL_STOP_PENDING on us and we'll re-enter
>
> This is that same else clause again; perhaps make signal_wake_up_state()
> also clear TRACED_XXX instead?
I swear, I too thought about this after my last email. Yes, I think this
makes sense. Thanks,
Oleg.
I was thinking about this and I have an approach from a different
direction. In particular it removes the need for ptrace_freeze_attach
and ptrace_unfreeze_attach to change __state. Instead a jobctl
bit is used to suppress waking up a process with TASK_WAKEKILL.
I think this would be a good technique to completely decouple
PREEMPT_RT from the work that ptrace_freeze_attach does.
Comments?
Eric
---
The games ptrace_freeze_traced and ptrace_unfreeze_traced have
been playing with __state to remove TASK_WAKEKILL from __state
while a ptrace command is executing.
Instead add JOBCTL_DELAY_WAKEILL and set it in ptrace_freeze_task and
clear it in ptrace_unfreeze_task and the final exit of ptrace_stop.
Then in signal_wake_up_state drop TASK_WAKEKILL if it is sent while
JOBCTL_DELAY_WAKEKILL is set.
As signal_wake_up is the only function that sets TASK_WAKEKILL when
waking a process this achives the same effect with less messing
with state.
Signed-off-by: "Eric W. Biederman" <[email protected]>
As a proof of concept this seems to work most of the time,
unfortunately I have a bug somewhere (a memory stomp?) and I am
hitting a BUG_ON that asserts __send_signal is not holding
the siglock.
[ 97.000168] ------------[ cut here ]------------
[ 97.001782] kernel BUG at kernel/signal.c:1086!
[ 97.002539] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[ 97.002846] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.18.0-rc1userns+ #449
[ 97.003135] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
[ 97.003480] RIP: 0010:__send_signal+0x256/0x580
[ 97.003812] Code: f0 ff ff 49 89 c7 48 85 c0 0f 85 a8 fe ff ff 41 bf 04 00 00 00 44 8d 45 ff e9 f8 fe ff ff 45 31 ff 40
[ 97.003812] RSP: 0018:ffffc900000c8e88 EFLAGS: 00000046
[ 97.003812] RAX: 0000000000000000 RBX: ffff88800875df00 RCX: 0000000000000001
[ 97.003812] RDX: ffff88800875df00 RSI: 0000000000000001 RDI: 000000000000000e
[ 97.003812] RBP: 000000000000000e R08: 0000000000000001 R09: 0000000000000000
[ 97.003812] R10: 0000000000000000 R11: ffffc900000c8ff8 R12: 0000000000000000
[ 97.003812] R13: 0000000000000001 R14: 0000000000000001 R15: ffffffff8115f3f3
[ 97.003812] FS: 0000000000000000(0000) GS:ffff8880bcb00000(0000) knlGS:0000000000000000
[ 97.003812] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 97.003812] CR2: 00007ffff739c830 CR3: 00000000094d6000 CR4: 00000000000006e0
[ 97.003812] Call Trace:
[ 97.003812] <IRQ>
[ 97.003812] group_send_sig_info+0xe1/0x190
[ 97.003812] kill_pid_info+0x78/0x150
[ 97.003812] it_real_fn+0x35/0xe0
[ 97.003812] ? __x64_sys_setitimer+0x150/0x150
[ 97.003812] __hrtimer_run_queues+0x1ca/0x3f0
[ 97.003812] hrtimer_interrupt+0x106/0x220
[ 97.003812] __sysvec_apic_timer_interrupt+0x96/0x260
[ 97.003812] sysvec_apic_timer_interrupt+0x9d/0xd0
[ 97.003812] </IRQ>
[ 97.003812] <TASK>
[ 97.003812] asm_sysvec_apic_timer_interrupt+0x12/0x20
[ 97.003812] RIP: 0010:default_idle+0x10/0x20
[ 97.003812] Code: 8b 04 25 00 b0 01 00 f0 80 60 02 df c3 0f ae f0 0f ae 38 0f ae f0 eb b9 66 90 0f 1f 44 00 00 eb 07 03
[ 97.003812] RSP: 0018:ffffc90000093ef8 EFLAGS: 00000246
[ 97.003812] RAX: ffffffff823fafb0 RBX: ffff8880056f97c0 RCX: 0000000000000000
[ 97.003812] RDX: ffffffff81193e1d RSI: ffffffff82da5301 RDI: ffffffff823fb111
[ 97.003812] RBP: 0000000000000001 R08: 0000000000000000 R09: 0000000000000000
[ 97.003812] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[ 97.003812] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 97.003812] ? mwait_idle+0x70/0x70
[ 97.003812] ? nohz_balance_enter_idle+0x6d/0x220
[ 97.003812] ? default_idle_call+0x21/0x90
[ 97.003812] ? mwait_idle+0x70/0x70
[ 97.003812] default_idle_call+0x59/0x90
[ 97.003812] do_idle+0x1f3/0x260
[ 97.003812] ? finish_task_switch.isra.0+0xef/0x350
[ 97.003812] cpu_startup_entry+0x19/0x20
[ 97.003812] secondary_startup_64_no_verify+0xc3/0xcb
[ 97.003812] </TASK>
[ 97.003812] Modules linked in:
[ 97.003812] ---[ end trace 0000000000000000 ]---
[ 97.003812] RIP: 0010:__send_signal+0x256/0x580
[ 97.003812] Code: f0 ff ff 49 89 c7 48 85 c0 0f 85 a8 fe ff ff 41 bf 04 00 00 00 44 8d 45 ff e9 f8 fe ff ff 45 31 ff 40
[ 97.003812] RSP: 0018:ffffc900000c8e88 EFLAGS: 00000046
[ 97.003812] RAX: 0000000000000000 RBX: ffff88800875df00 RCX: 0000000000000001
[ 97.003812] RDX: ffff88800875df00 RSI: 0000000000000001 RDI: 000000000000000e
[ 97.003812] RBP: 000000000000000e R08: 0000000000000001 R09: 0000000000000000
[ 97.003812] R10: 0000000000000000 R11: ffffc900000c8ff8 R12: 0000000000000000
[ 97.003812] R13: 0000000000000001 R14: 0000000000000001 R15: ffffffff8115f3f3
[ 97.003812] FS: 0000000000000000(0000) GS:ffff8880bcb00000(0000) knlGS:0000000000000000
[ 97.003812] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 97.003812] CR2: 00007ffff739c830 CR3: 00000000094d6000 CR4: 00000000000006e0
[ 97.003812] Kernel panic - not syncing: Fatal exception in interrupt
[ 97.003812] Kernel Offset: disabled
[ 97.003812] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---
---
include/linux/sched/jobctl.h | 2 ++
kernel/ptrace.c | 32 ++++++++++++++------------------
kernel/signal.c | 5 +++++
3 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/include/linux/sched/jobctl.h b/include/linux/sched/jobctl.h
index fa067de9f1a9..4e154ad8205f 100644
--- a/include/linux/sched/jobctl.h
+++ b/include/linux/sched/jobctl.h
@@ -19,6 +19,7 @@ struct task_struct;
#define JOBCTL_TRAPPING_BIT 21 /* switching to TRACED */
#define JOBCTL_LISTENING_BIT 22 /* ptracer is listening for events */
#define JOBCTL_TRAP_FREEZE_BIT 23 /* trap for cgroup freezer */
+#define JOBCTL_DELAY_WAKEKILL_BIT 24 /* delay killable wakeups */
#define JOBCTL_STOP_DEQUEUED (1UL << JOBCTL_STOP_DEQUEUED_BIT)
#define JOBCTL_STOP_PENDING (1UL << JOBCTL_STOP_PENDING_BIT)
@@ -28,6 +29,7 @@ struct task_struct;
#define JOBCTL_TRAPPING (1UL << JOBCTL_TRAPPING_BIT)
#define JOBCTL_LISTENING (1UL << JOBCTL_LISTENING_BIT)
#define JOBCTL_TRAP_FREEZE (1UL << JOBCTL_TRAP_FREEZE_BIT)
+#define JOBCTL_DELAY_WAKEKILL (1UL << JOBCTL_DELAY_WAKEKILL_BIT)
#define JOBCTL_TRAP_MASK (JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)
#define JOBCTL_PENDING_MASK (JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 4f78d0d5940c..a55320a0e8e3 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -197,7 +197,8 @@ static bool ptrace_freeze_traced(struct task_struct *task)
spin_lock_irq(&task->sighand->siglock);
if (task_is_traced(task) && !looks_like_a_spurious_pid(task) &&
!__fatal_signal_pending(task)) {
- WRITE_ONCE(task->__state, __TASK_TRACED);
+ //WARN_ON((task->jobctl & JOBCTL_DELAY_WAKEKILL));
+ task->jobctl |= JOBCTL_DELAY_WAKEKILL;
ret = true;
}
spin_unlock_irq(&task->sighand->siglock);
@@ -207,7 +208,7 @@ static bool ptrace_freeze_traced(struct task_struct *task)
static void ptrace_unfreeze_traced(struct task_struct *task)
{
- if (READ_ONCE(task->__state) != __TASK_TRACED)
+ if (!task_is_traced(task))
return;
WARN_ON(!task->ptrace || task->parent != current);
@@ -216,13 +217,11 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
* PTRACE_LISTEN can allow ptrace_trap_notify to wake us up remotely.
* Recheck state under the lock to close this race.
*/
- spin_lock_irq(&task->sighand->siglock);
- if (READ_ONCE(task->__state) == __TASK_TRACED) {
- if (__fatal_signal_pending(task))
- wake_up_state(task, __TASK_TRACED);
- else
- WRITE_ONCE(task->__state, TASK_TRACED);
- }
+ spin_unlock_irq(&task->sighand->siglock);
+ WARN_ON(!(task->jobctl & JOBCTL_DELAY_WAKEKILL));
+ task->jobctl &= ~JOBCTL_DELAY_WAKEKILL;
+ if (fatal_signal_pending(task))
+ wake_up_state(task, TASK_WAKEKILL);
spin_unlock_irq(&task->sighand->siglock);
}
@@ -256,7 +255,7 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
*/
read_lock(&tasklist_lock);
if (child->ptrace && child->parent == current) {
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
+ WARN_ON(child->jobctl & JOBCTL_DELAY_WAKEKILL);
/*
* child->sighand can't be NULL, release_task()
* does ptrace_unlink() before __exit_signal().
@@ -267,13 +266,13 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
read_unlock(&tasklist_lock);
if (!ret && !ignore_state) {
- if (!wait_task_inactive(child, __TASK_TRACED)) {
+ if (!wait_task_inactive(child, TASK_TRACED)) {
/*
* This can only happen if may_ptrace_stop() fails and
* ptrace_stop() changes ->state back to TASK_RUNNING,
- * so we should not worry about leaking __TASK_TRACED.
+ * so we should not worry about leaking JOBCTL_DELAY_WAKEKILL.
*/
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
+ WARN_ON(!(child->jobctl & JOBCTL_DELAY_WAKEKILL));
ret = -ESRCH;
}
}
@@ -892,7 +891,6 @@ static int ptrace_resume(struct task_struct *child, long request,
* status and clears the code too; this can't race with the tracee, it
* takes siglock after resume.
*/
- need_siglock = data && !thread_group_empty(current);
if (need_siglock)
spin_lock_irq(&child->sighand->siglock);
child->exit_code = data;
@@ -1325,8 +1323,7 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
goto out_put_task_struct;
ret = arch_ptrace(child, request, addr, data);
- if (ret || request != PTRACE_DETACH)
- ptrace_unfreeze_traced(child);
+ ptrace_unfreeze_traced(child);
out_put_task_struct:
put_task_struct(child);
@@ -1468,8 +1465,7 @@ COMPAT_SYSCALL_DEFINE4(ptrace, compat_long_t, request, compat_long_t, pid,
request == PTRACE_INTERRUPT);
if (!ret) {
ret = compat_arch_ptrace(child, request, addr, data);
- if (ret || request != PTRACE_DETACH)
- ptrace_unfreeze_traced(child);
+ ptrace_unfreeze_traced(child);
}
out_put_task_struct:
diff --git a/kernel/signal.c b/kernel/signal.c
index 30cd1ca43bcd..a1277580c92e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -762,6 +762,10 @@ static int dequeue_synchronous_signal(kernel_siginfo_t *info)
*/
void signal_wake_up_state(struct task_struct *t, unsigned int state)
{
+ /* Suppress wakekill? */
+ if (t->jobctl & JOBCTL_DELAY_WAKEKILL)
+ state &= ~TASK_WAKEKILL;
+
set_tsk_thread_flag(t, TIF_SIGPENDING);
/*
* TASK_WAKEKILL also means wake it up in the stopped/traced/killable
@@ -2328,6 +2332,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
/* LISTENING can be set only during STOP traps, clear it */
current->jobctl &= ~JOBCTL_LISTENING;
+ current->jobctl &= ~JOBCTL_DELAY_WAKEKILL;
/*
* Queued signals ignored us while we were stopped for tracing.
--
2.35.3
On Thu, Apr 21, 2022 at 12:49:43PM +0200, Oleg Nesterov wrote:
> On 04/21, Peter Zijlstra wrote:
> >
> > As such, I've taken the liberty of munging yours and Oleg's approach
> > together. I've yet to actually test this but it looks feasible.
>
> Agreed, but I am a but confused. Is it on top of 1/5? Doesn't look so,
> and I can't apply it with or without 1/5. So I am not sure I understand
> what exactly it does.
Yes, it is on top of a modified 1, I mean to post an updated and tested
series later today.
Basically it does the TRACED_QUIESCE thing from your patch, and then
does the DELAY_WAKEKILL thing from Eric's patch.
On 04/20, Eric W. Biederman wrote:
>
> I was thinking about this and I have an approach from a different
> direction. In particular it removes the need for ptrace_freeze_attach
> and ptrace_unfreeze_attach to change __state. Instead a jobctl
> bit is used to suppress waking up a process with TASK_WAKEKILL.
I think this can work, but we still need something like 1/5 + 2/5?
> I think this would be a good technique to completely decouple
> PREEMPT_RT from the work that ptrace_freeze_attach does.
If CONFIG_RT=y we can't rely on the ->__state check in task_is_traced(),
and wait_task_inactive() can wrongly fail if the tracee sleeps waiting
for tasklist_lock.
A couple of comments after a quick glance,
> static void ptrace_unfreeze_traced(struct task_struct *task)
> {
> - if (READ_ONCE(task->__state) != __TASK_TRACED)
> + if (!task_is_traced(task))
> return;
>
> WARN_ON(!task->ptrace || task->parent != current);
> @@ -216,13 +217,11 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
> * PTRACE_LISTEN can allow ptrace_trap_notify to wake us up remotely.
> * Recheck state under the lock to close this race.
> */
> - spin_lock_irq(&task->sighand->siglock);
> - if (READ_ONCE(task->__state) == __TASK_TRACED) {
> - if (__fatal_signal_pending(task))
> - wake_up_state(task, __TASK_TRACED);
> - else
> - WRITE_ONCE(task->__state, TASK_TRACED);
> - }
> + spin_unlock_irq(&task->sighand->siglock);
> + WARN_ON(!(task->jobctl & JOBCTL_DELAY_WAKEKILL));
> + task->jobctl &= ~JOBCTL_DELAY_WAKEKILL;
We can't rely on the lockless task_is_traced() check above... probably this
is fine, but I need to re-chesk. But at least you need to remove the comment
about PTRACE_LISTEN above.
Another problem is that WARN_ON(!(task->jobctl & JOBCTL_DELAY_WAKEKILL))
doesn't look right if ignore_state in ptrace_check_attach() was true, the
tracee could stop before ptrace_unfreeze_traced().
> @@ -892,7 +891,6 @@ static int ptrace_resume(struct task_struct *child, long request,
> * status and clears the code too; this can't race with the tracee, it
> * takes siglock after resume.
> */
> - need_siglock = data && !thread_group_empty(current);
> if (need_siglock)
> spin_lock_irq(&child->sighand->siglock);
Hmm?
Oleg.
Oleg Nesterov <[email protected]> writes:
> On 04/20, Eric W. Biederman wrote:
>> @@ -892,7 +891,6 @@ static int ptrace_resume(struct task_struct *child, long request,
>> * status and clears the code too; this can't race with the tracee, it
>> * takes siglock after resume.
>> */
>> - need_siglock = data && !thread_group_empty(current);
>> if (need_siglock)
>> spin_lock_irq(&child->sighand->siglock);
>
> Hmm?
A half backed out change (I thought ptrace_resume would need to clear
JOBCTL_DELAY_WAKEKILL) in ptrace_resume. I somehow failed to
restore the need_siglock line.
Eric
On 04/18, Oleg Nesterov wrote:
>
> static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
> {
...
> + if (!traced)
> + return -ESRCH;
> +
> + WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
> + if (ignore_state)
> + return 0;
> +
> + if (!task_is_traced(child))
> + return -ESRCH;
This is the new check V1 didn't have, we need it to unsure that check_attach
can't miss the change in child->jobctl and call wait_task_inactive() before
the child marks itself as "traced" and clears JOBCTL_TRACED_XXX.
Oleg.
On Wed, Apr 20, 2022 at 03:54:15PM -0500, Eric W. Biederman wrote:
>
> I was thinking about this and I have an approach from a different
> direction. In particular it removes the need for ptrace_freeze_attach
> and ptrace_unfreeze_attach to change __state. Instead a jobctl
> bit is used to suppress waking up a process with TASK_WAKEKILL.
>
> I think this would be a good technique to completely decouple
> PREEMPT_RT from the work that ptrace_freeze_attach does.
>
> Comments?
On first read-through, I like it! A few comments down below..
> @@ -216,13 +217,11 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
> * PTRACE_LISTEN can allow ptrace_trap_notify to wake us up remotely.
> * Recheck state under the lock to close this race.
> */
> - spin_lock_irq(&task->sighand->siglock);
> - if (READ_ONCE(task->__state) == __TASK_TRACED) {
> - if (__fatal_signal_pending(task))
> - wake_up_state(task, __TASK_TRACED);
> - else
> - WRITE_ONCE(task->__state, TASK_TRACED);
> - }
> + spin_unlock_irq(&task->sighand->siglock);
^^^^ this should be spin_lock_irq(...)
> + WARN_ON(!(task->jobctl & JOBCTL_DELAY_WAKEKILL));
> + task->jobctl &= ~JOBCTL_DELAY_WAKEKILL;
> + if (fatal_signal_pending(task))
> + wake_up_state(task, TASK_WAKEKILL);
> spin_unlock_irq(&task->sighand->siglock);
> }
>
> @@ -256,7 +255,7 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
> */
> read_lock(&tasklist_lock);
> if (child->ptrace && child->parent == current) {
> - WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
> + WARN_ON(child->jobctl & JOBCTL_DELAY_WAKEKILL);
> /*
> * child->sighand can't be NULL, release_task()
> * does ptrace_unlink() before __exit_signal().
> @@ -267,13 +266,13 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
> read_unlock(&tasklist_lock);
>
> if (!ret && !ignore_state) {
> - if (!wait_task_inactive(child, __TASK_TRACED)) {
> + if (!wait_task_inactive(child, TASK_TRACED)) {
This is still very dubious, there are spinlocks between
set_current_state(TASK_TRACED) and schedule(), so wait_task_inactive()
can fail where we don't want it to due to TASK_TRACED being temporarily
held in ->saved_state.
> /*
> * This can only happen if may_ptrace_stop() fails and
> * ptrace_stop() changes ->state back to TASK_RUNNING,
> - * so we should not worry about leaking __TASK_TRACED.
> + * so we should not worry about leaking JOBCTL_DELAY_WAKEKILL.
> */
> + WARN_ON(!(child->jobctl & JOBCTL_DELAY_WAKEKILL));
> ret = -ESRCH;
> }
> }
On 04/20, Peter Zijlstra wrote:
>
> On Fri, Apr 15, 2022 at 12:57:56PM +0200, Oleg Nesterov wrote:
> >
> > ptrace_resume() does wake_up_state(child, __TASK_TRACED) but doesn't
> > clear JOBCTL_TRACED. The "else" branch in ptrace_stop() leaks this flag
> > too. Perhaps I missed something, I'll reread 1/5 again, but the main
> > question to me is whether 1-2 actually need the JOBCTL_TRACED_FROZEN flag.
>
> Ok, getting back to this. So I did the change to ptrace_resume(), but
> I'm not entirely sure I understand the issue with the else branch of
> ptrace_stop().
>
> My understanding is that if we hit that else branch, we've raced wth
> __ptrace_unlink(), and that will have done:
Yes, thanks for correcting me, I forgot that may_ptrace_stop() has gone.
Oleg.
On Thu, Apr 21, 2022 at 09:21:38AM +0200, Peter Zijlstra wrote:
> > @@ -267,13 +266,13 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
> > read_unlock(&tasklist_lock);
> >
> > if (!ret && !ignore_state) {
> > - if (!wait_task_inactive(child, __TASK_TRACED)) {
> > + if (!wait_task_inactive(child, TASK_TRACED)) {
>
> This is still very dubious, there are spinlocks between
> set_current_state(TASK_TRACED) and schedule(), so wait_task_inactive()
> can fail where we don't want it to due to TASK_TRACED being temporarily
> held in ->saved_state.
As such, I've taken the liberty of munging yours and Oleg's approach
together. I've yet to actually test this but it looks feasible.
---
--- a/include/linux/sched/jobctl.h
+++ b/include/linux/sched/jobctl.h
@@ -19,9 +19,11 @@ struct task_struct;
#define JOBCTL_TRAPPING_BIT 21 /* switching to TRACED */
#define JOBCTL_LISTENING_BIT 22 /* ptracer is listening for events */
#define JOBCTL_TRAP_FREEZE_BIT 23 /* trap for cgroup freezer */
+#define JOBCTL_DELAY_WAKEKILL_BIT 24 /* delay killable wakeups */
-#define JOBCTL_STOPPED_BIT 24
-#define JOBCTL_TRACED_BIT 25
+#define JOBCTL_STOPPED_BIT 25
+#define JOBCTL_TRACED_BIT 26
+#define JOBCTL_TRACED_QUIESCE_BIT 27
#define JOBCTL_STOP_DEQUEUED (1UL << JOBCTL_STOP_DEQUEUED_BIT)
#define JOBCTL_STOP_PENDING (1UL << JOBCTL_STOP_PENDING_BIT)
@@ -31,9 +33,11 @@ struct task_struct;
#define JOBCTL_TRAPPING (1UL << JOBCTL_TRAPPING_BIT)
#define JOBCTL_LISTENING (1UL << JOBCTL_LISTENING_BIT)
#define JOBCTL_TRAP_FREEZE (1UL << JOBCTL_TRAP_FREEZE_BIT)
+#define JOBCTL_DELAY_WAKEKILL (1UL << JOBCTL_DELAY_WAKEKILL_BIT)
#define JOBCTL_STOPPED (1UL << JOBCTL_STOPPED_BIT)
#define JOBCTL_TRACED (1UL << JOBCTL_TRACED_BIT)
+#define JOBCTL_TRACED_QUIESCE (1UL << JOBCTL_TRACED_QUIESCE_BIT)
#define JOBCTL_TRAP_MASK (JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)
#define JOBCTL_PENDING_MASK (JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -193,26 +193,32 @@ static bool looks_like_a_spurious_pid(st
*/
static bool ptrace_freeze_traced(struct task_struct *task)
{
+ unsigned long flags;
bool ret = false;
/* Lockless, nobody but us can set this flag */
if (task->jobctl & JOBCTL_LISTENING)
return ret;
- spin_lock_irq(&task->sighand->siglock);
- if (task_is_traced(task) && !looks_like_a_spurious_pid(task) &&
+ if (!lock_task_sighand(task, &flags))
+ return ret;
+
+ if (READ_ONCE(task->__state) == TASK_TRACED &&
+ !looks_like_a_spurious_pid(task) &&
!__fatal_signal_pending(task)) {
- WRITE_ONCE(task->__state, __TASK_TRACED);
+ WARN_ON_ONCE(!task_is_traced(task));
+ WARN_ON_ONCE(task->jobctl & JOBCTL_DELAY_WAKEKILL);
+ task->jobctl |= JOBCTL_DELAY_WAKEKILL;
ret = true;
}
- spin_unlock_irq(&task->sighand->siglock);
+ unlock_task_sighand(task, &flags);
return ret;
}
static void ptrace_unfreeze_traced(struct task_struct *task)
{
- if (READ_ONCE(task->__state) != __TASK_TRACED)
+ if (!task_is_traced(task))
return;
WARN_ON(!task->ptrace || task->parent != current);
@@ -222,12 +228,11 @@ static void ptrace_unfreeze_traced(struc
* Recheck state under the lock to close this race.
*/
spin_lock_irq(&task->sighand->siglock);
- if (READ_ONCE(task->__state) == __TASK_TRACED) {
- if (__fatal_signal_pending(task)) {
- task->jobctl &= ~JOBCTL_TRACED;
- wake_up_state(task, __TASK_TRACED);
- } else
- WRITE_ONCE(task->__state, TASK_TRACED);
+// WARN_ON_ONCE(!(task->jobctl & JOBCTL_DELAY_WAKEKILL));
+ task->jobctl &= ~JOBCTL_DELAY_WAKEKILL;
+ if (__fatal_signal_pending(task)) {
+ task->jobctl &= ~JOBCTL_TRACED;
+ wake_up_state(task, TASK_WAKEKILL);
}
spin_unlock_irq(&task->sighand->siglock);
}
@@ -251,40 +256,46 @@ static void ptrace_unfreeze_traced(struc
*/
static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
{
- int ret = -ESRCH;
+ int traced;
/*
* We take the read lock around doing both checks to close a
- * possible race where someone else was tracing our child and
- * detached between these two checks. After this locked check,
- * we are sure that this is our traced child and that can only
- * be changed by us so it's not changing right after this.
+ * possible race where someone else attaches or detaches our
+ * natural child.
*/
read_lock(&tasklist_lock);
- if (child->ptrace && child->parent == current) {
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
- /*
- * child->sighand can't be NULL, release_task()
- * does ptrace_unlink() before __exit_signal().
- */
- if (ignore_state || ptrace_freeze_traced(child))
- ret = 0;
- }
+ traced = child->ptrace && child->parent == current;
read_unlock(&tasklist_lock);
+ if (!traced)
+ return -ESRCH;
- if (!ret && !ignore_state) {
- if (!wait_task_inactive(child, __TASK_TRACED)) {
- /*
- * This can only happen if may_ptrace_stop() fails and
- * ptrace_stop() changes ->state back to TASK_RUNNING,
- * so we should not worry about leaking __TASK_TRACED.
- */
- WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
- ret = -ESRCH;
- }
+ WARN_ON_ONCE(READ_ONCE(child->__state) == __TASK_TRACED);
+ WARN_ON_ONCE(READ_ONCE(child->jobctl) & JOBCTL_DELAY_WAKEKILL);
+
+ if (ignore_state)
+ return 0;
+
+ if (!task_is_traced(child))
+ return -ESRCH;
+
+ /* wait for JOBCTL_TRACED_QUIESCE to go away, see ptrace_stop() */
+ for (;;) {
+ if (fatal_signal_pending(current))
+ return -EINTR;
+
+ set_current_state(TASK_KILLABLE);
+ if (!(READ_ONCE(child->jobctl) & JOBCTL_TRACED_QUIESCE))
+ break;
+
+ schedule();
}
+ __set_current_state(TASK_RUNNING);
- return ret;
+ if (!wait_task_inactive(child, TASK_TRACED) ||
+ !ptrace_freeze_traced(child))
+ return -ESRCH;
+
+ return 0;
}
static bool ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
@@ -1329,8 +1340,7 @@ SYSCALL_DEFINE4(ptrace, long, request, l
goto out_put_task_struct;
ret = arch_ptrace(child, request, addr, data);
- if (ret || request != PTRACE_DETACH)
- ptrace_unfreeze_traced(child);
+ ptrace_unfreeze_traced(child);
out_put_task_struct:
put_task_struct(child);
@@ -1472,8 +1482,7 @@ COMPAT_SYSCALL_DEFINE4(ptrace, compat_lo
request == PTRACE_INTERRUPT);
if (!ret) {
ret = compat_arch_ptrace(child, request, addr, data);
- if (ret || request != PTRACE_DETACH)
- ptrace_unfreeze_traced(child);
+ ptrace_unfreeze_traced(child);
}
out_put_task_struct:
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -764,6 +764,10 @@ void signal_wake_up_state(struct task_st
{
lockdep_assert_held(&t->sighand->siglock);
+ /* Suppress wakekill? */
+ if (t->jobctl & JOBCTL_DELAY_WAKEKILL)
+ state &= ~TASK_WAKEKILL;
+
set_tsk_thread_flag(t, TIF_SIGPENDING);
/*
@@ -774,7 +778,7 @@ void signal_wake_up_state(struct task_st
* handle its death signal.
*/
if (wake_up_state(t, state | TASK_INTERRUPTIBLE))
- t->jobctl &= ~(JOBCTL_STOPPED | JOBCTL_TRACED);
+ t->jobctl &= ~(JOBCTL_STOPPED | JOBCTL_TRACED | JOBCTL_TRACED_QUIESCE);
else
kick_process(t);
}
@@ -2187,6 +2191,15 @@ static void do_notify_parent_cldstop(str
spin_unlock_irqrestore(&sighand->siglock, flags);
}
+static void clear_traced_quiesce(void)
+{
+ spin_lock_irq(¤t->sighand->siglock);
+ WARN_ON_ONCE(!(current->jobctl & JOBCTL_TRACED_QUIESCE));
+ current->jobctl &= ~JOBCTL_TRACED_QUIESCE;
+ wake_up_state(current->parent, TASK_KILLABLE);
+ spin_unlock_irq(¤t->sighand->siglock);
+}
+
/*
* This must be called with current->sighand->siglock held.
*
@@ -2225,7 +2238,7 @@ static int ptrace_stop(int exit_code, in
* schedule() will not sleep if there is a pending signal that
* can awaken the task.
*/
- current->jobctl |= JOBCTL_TRACED;
+ current->jobctl |= JOBCTL_TRACED | JOBCTL_TRACED_QUIESCE;
set_special_state(TASK_TRACED);
/*
@@ -2290,14 +2303,26 @@ static int ptrace_stop(int exit_code, in
/*
* Don't want to allow preemption here, because
* sys_ptrace() needs this task to be inactive.
- *
- * XXX: implement read_unlock_no_resched().
*/
preempt_disable();
read_unlock(&tasklist_lock);
- cgroup_enter_frozen();
+ cgroup_enter_frozen(); // XXX broken on PREEMPT_RT !!!
+
+ /*
+ * JOBCTL_TRACE_QUIESCE bridges the gap between
+ * set_current_state(TASK_TRACED) above and schedule() below.
+ * There must not be any blocking (specifically anything that
+ * touched ->saved_state on PREEMPT_RT) between here and
+ * schedule().
+ *
+ * ptrace_check_attach() relies on this with its
+ * wait_task_inactive() usage.
+ */
+ clear_traced_quiesce();
+
preempt_enable_no_resched();
freezable_schedule();
+
cgroup_leave_frozen(true);
} else {
/*
@@ -2335,6 +2360,7 @@ static int ptrace_stop(int exit_code, in
/* LISTENING can be set only during STOP traps, clear it */
current->jobctl &= ~JOBCTL_LISTENING;
+ current->jobctl &= ~JOBCTL_DELAY_WAKEKILL;
/*
* Queued signals ignored us while we were stopped for tracing.
On Mon, Apr 18, 2022 at 07:01:05PM +0200, Oleg Nesterov wrote:
> diff --git a/include/linux/sched/jobctl.h b/include/linux/sched/jobctl.h
> index ec8b312f7506..1b5a57048e13 100644
> --- a/include/linux/sched/jobctl.h
> +++ b/include/linux/sched/jobctl.h
> @@ -22,7 +22,8 @@ struct task_struct;
>
> #define JOBCTL_STOPPED_BIT 24
> #define JOBCTL_TRACED_BIT 25
> +#define JOBCTL_TRACED_XXX_BIT 25
26, also we must come up with a better name than tripple-x. In my head
it's started to be called TRACED_OLEG, but that can't be right either
;-)
Does something like:
#define JOBCTL_TRACED_BIT 25
#define JOBCTL_TRACED_QUIESCE_BIT 26
work?
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 0aea3f0a8002..c7a89904cc4a 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2182,6 +2182,13 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
> spin_unlock_irqrestore(&sighand->siglock, flags);
> }
>
> +static void clear_traced_xxx(void)
> +{
> + spin_lock_irq(¤t->sighand->siglock);
> + current->jobctl &= ~JOBCTL_TRACED_XXX;
> + spin_unlock_irq(¤t->sighand->siglock);
> +}
> +
> /*
> * This must be called with current->sighand->siglock held.
> *
> @@ -2220,7 +2227,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> * schedule() will not sleep if there is a pending signal that
> * can awaken the task.
> */
> - current->jobctl |= JOBCTL_TRACED;
> + current->jobctl |= JOBCTL_TRACED | JOBCTL_TRACED_XXX;
> set_special_state(TASK_TRACED);
>
> /*
> @@ -2282,6 +2289,8 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> if (gstop_done && ptrace_reparented(current))
> do_notify_parent_cldstop(current, false, why);
>
> + clear_traced_xxx();
> + wake_up_state(current->parent, TASK_KILLABLE);
> /*
> * Don't want to allow preemption here, because
> * sys_ptrace() needs this task to be inactive.
> @@ -2297,8 +2306,12 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> } else {
> /*
> * By the time we got the lock, our tracer went away.
> - * Don't drop the lock yet, another tracer may come.
> - *
> + * Don't drop the lock yet, another tracer may come,
> + * tasklist protects us from ptrace_freeze_traced().
> + */
> + __set_current_state(TASK_RUNNING);
> + clear_traced_xxx();
> + /*
> * If @gstop_done, the ptracer went away between group stop
> * completion and here. During detach, it would have set
> * JOBCTL_STOP_PENDING on us and we'll re-enter
This is that same else clause again; perhaps make signal_wake_up_state()
also clear TRACED_XXX instead?
Peter Zijlstra <[email protected]> writes:
> On Wed, Apr 20, 2022 at 03:54:15PM -0500, Eric W. Biederman wrote:
>>
>> I was thinking about this and I have an approach from a different
>> direction. In particular it removes the need for ptrace_freeze_attach
>> and ptrace_unfreeze_attach to change __state. Instead a jobctl
>> bit is used to suppress waking up a process with TASK_WAKEKILL.
>>
>> I think this would be a good technique to completely decouple
>> PREEMPT_RT from the work that ptrace_freeze_attach does.
>>
>> Comments?
>
> On first read-through, I like it! A few comments down below..
>
>> @@ -216,13 +217,11 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
>> * PTRACE_LISTEN can allow ptrace_trap_notify to wake us up remotely.
>> * Recheck state under the lock to close this race.
>> */
>> - spin_lock_irq(&task->sighand->siglock);
>> - if (READ_ONCE(task->__state) == __TASK_TRACED) {
>> - if (__fatal_signal_pending(task))
>> - wake_up_state(task, __TASK_TRACED);
>> - else
>> - WRITE_ONCE(task->__state, TASK_TRACED);
>> - }
>> + spin_unlock_irq(&task->sighand->siglock);
>
> ^^^^ this should be spin_lock_irq(...)
Doh!
Thank you for spotting that. That solves my nasty splat in
__send_signal.
>
>> + WARN_ON(!(task->jobctl & JOBCTL_DELAY_WAKEKILL));
>> + task->jobctl &= ~JOBCTL_DELAY_WAKEKILL;
>> + if (fatal_signal_pending(task))
>> + wake_up_state(task, TASK_WAKEKILL);
>> spin_unlock_irq(&task->sighand->siglock);
>> }
>>
>> @@ -256,7 +255,7 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
>> */
>> read_lock(&tasklist_lock);
>> if (child->ptrace && child->parent == current) {
>> - WARN_ON(READ_ONCE(child->__state) == __TASK_TRACED);
>> + WARN_ON(child->jobctl & JOBCTL_DELAY_WAKEKILL);
>> /*
>> * child->sighand can't be NULL, release_task()
>> * does ptrace_unlink() before __exit_signal().
>> @@ -267,13 +266,13 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
>> read_unlock(&tasklist_lock);
>>
>> if (!ret && !ignore_state) {
>> - if (!wait_task_inactive(child, __TASK_TRACED)) {
>> + if (!wait_task_inactive(child, TASK_TRACED)) {
>
> This is still very dubious, there are spinlocks between
> set_current_state(TASK_TRACED) and schedule(), so wait_task_inactive()
> can fail where we don't want it to due to TASK_TRACED being temporarily
> held in ->saved_state.
When it comes to PREEMPT_RT yes.
I think we might be able to remove the wait_task_inactive, I am
not certain what it gets us.
All this change gets us is the removal of playing with __state.
Eric
On 04/21, Peter Zijlstra wrote:
>
> As such, I've taken the liberty of munging yours and Oleg's approach
> together. I've yet to actually test this but it looks feasible.
Agreed, but I am a but confused. Is it on top of 1/5? Doesn't look so,
and I can't apply it with or without 1/5. So I am not sure I understand
what exactly it does.
Oleg.
On Fri, Apr 15, 2022 at 12:57:56PM +0200, Oleg Nesterov wrote:
> On 04/15, Oleg Nesterov wrote:
> >
> > 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() ?
> >
>
> Forgot to say, I think 1/5 needs some changes in any case...
>
> ptrace_resume() does wake_up_state(child, __TASK_TRACED) but doesn't
> clear JOBCTL_TRACED. The "else" branch in ptrace_stop() leaks this flag
> too. Perhaps I missed something, I'll reread 1/5 again, but the main
> question to me is whether 1-2 actually need the JOBCTL_TRACED_FROZEN flag.
Ok, getting back to this. So I did the change to ptrace_resume(), but
I'm not entirely sure I understand the issue with the else branch of
ptrace_stop().
My understanding is that if we hit that else branch, we've raced wth
__ptrace_unlink(), and that will have done:
if (... || task_is_traced(child))
ptrace_signal_wake_up(child, true);
Which will have done that wakeup and cleared both __state and jobctl.
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: 7d3fafb75102c0e8d5282487c2822d0f3b301aa9 ("[RFC][PATCH] ptrace: Don't change __state")
url: https://github.com/intel-lab-lkp/linux/commits/Eric-W-Biederman/ptrace-Don-t-change-__state/20220421-045703
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git b253435746d9a4a701b5f09211b9c14d3370d0da
patch link: https://lore.kernel.org/lkml/[email protected]
in testcase: boot
on test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 4G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+------------------------------------------------------------------------------+------------+------------+
| | b253435746 | 7d3fafb751 |
+------------------------------------------------------------------------------+------------+------------+
| boot_successes | 29 | 4 |
| boot_failures | 0 | 35 |
| BUG:sleeping_function_called_from_invalid_context_at_arch/x86/entry/common.c | 0 | 35 |
| BUG:scheduling_while_atomic | 0 | 32 |
| WARNING:at_arch/x86/include/asm/uaccess.h:#__do_sys_waitid | 0 | 16 |
| EIP:__do_sys_waitid | 0 | 16 |
| WARNING:at_lib/usercopy.c:#_copy_to_user | 0 | 33 |
| EIP:_copy_to_user | 0 | 33 |
| WARNING:at_lib/usercopy.c:#_copy_from_user | 0 | 16 |
| EIP:_copy_from_user | 0 | 16 |
| WARNING:at_fs/read_write.c:#vfs_write | 0 | 16 |
| EIP:vfs_write | 0 | 16 |
| WARNING:at_arch/x86/include/asm/uaccess.h:#fault_in_readable | 0 | 16 |
| EIP:fault_in_readable | 0 | 16 |
| WARNING:at_arch/x86/include/asm/uaccess.h:#strncpy_from_user | 0 | 17 |
| EIP:strncpy_from_user | 0 | 17 |
| WARNING:at_lib/iov_iter.c:#__import_iovec | 0 | 6 |
| EIP:__import_iovec | 0 | 6 |
| WARNING:at_lib/iov_iter.c:#copyin | 0 | 5 |
| EIP:copyin | 0 | 5 |
| WARNING:at_fs/read_write.c:#vfs_read | 0 | 12 |
| EIP:vfs_read | 0 | 12 |
| WARNING:at_arch/x86/include/asm/uaccess.h:#fault_in_writeable | 0 | 12 |
| EIP:fault_in_writeable | 0 | 12 |
| WARNING:at_net/core/skbuff.c:#skb_release_head_state | 0 | 4 |
| EIP:skb_release_head_state | 0 | 4 |
| WARNING:at_arch/x86/kernel/fpu/signal.c:#copy_fpstate_to_sigframe | 0 | 24 |
| EIP:copy_fpstate_to_sigframe | 0 | 24 |
| WARNING:at_arch/x86/include/asm/uaccess.h:#__setup_frame | 0 | 24 |
| EIP:__setup_frame | 0 | 24 |
| WARNING:at_arch/x86/kernel/signal.c:#__do_sys_sigreturn | 0 | 7 |
| EIP:__do_sys_sigreturn | 0 | 7 |
| WARNING:at_arch/x86/kernel/fpu/signal.c:#fpu__restore_sig | 0 | 7 |
| EIP:fpu__restore_sig | 0 | 7 |
| WARNING:at_kernel/futex/core.c:#get_futex_key | 0 | 2 |
| EIP:get_futex_key | 0 | 2 |
| Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= | 0 | 19 |
| WARNING:at_mm/highmem.c:#__kmap_local_pfn_prot | 0 | 9 |
| EIP:__kmap_local_pfn_prot | 0 | 9 |
| WARNING:at_arch/x86/include/asm/uaccess.h:#do_sys_poll | 0 | 6 |
| EIP:do_sys_poll | 0 | 6 |
| WARNING:at_lib/iov_iter.c:#copyout | 0 | 2 |
| EIP:copyout | 0 | 2 |
| kernel_BUG_at_mm/vmalloc.c | 0 | 14 |
| invalid_opcode:#[##] | 0 | 14 |
| EIP:__get_vm_area_node | 0 | 14 |
| Kernel_panic-not_syncing:Fatal_exception_in_interrupt | 0 | 14 |
| WARNING:at_lib/iov_iter.c:#import_single_range | 0 | 1 |
| EIP:import_single_range | 0 | 1 |
+------------------------------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 7.186422][ C0] BUG: sleeping function called from invalid context at arch/x86/entry/common.c:161
[ 7.186426][ C0] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: init
[ 7.186427][ C0] preempt_count: 7ffffffe, expected: 0
[ 7.186430][ C0] CPU: 0 PID: 1 Comm: init Not tainted 5.18.0-rc3-00017-g7d3fafb75102 #1
[ 7.186433][ C0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 7.186434][ C0] Call Trace:
[ 7.186436][ C0] ? show_stack+0x3d/0x45
[ 7.186446][ C0] dump_stack_lvl+0x34/0x44
[ 7.186452][ C0] dump_stack+0xd/0x10
[ 7.186454][ C0] __might_resched.cold+0x9c/0xad
[ 7.186460][ C0] __might_sleep+0x33/0x80
[ 7.186464][ C0] __might_fault+0x28/0x40
[ 7.186468][ C0] __do_fast_syscall_32+0x25/0x100
[ 7.186473][ C0] do_fast_syscall_32+0x29/0x80
[ 7.186476][ C0] do_SYSENTER_32+0x15/0x40
[ 7.186479][ C0] entry_SYSENTER_32+0x98/0xf1
[ 7.186484][ C0] EIP: 0xb7f7c589
[ 7.186486][ C0] Code: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d 76 00 58 b8 77 00 00 00 cd 80 90 8d 76
[ 7.186488][ C0] EAX: ffffffda EBX: 00000007 ECX: 00000056 EDX: 00000000
[ 7.186490][ C0] ESI: 00000000 EDI: b7ef9ff4 EBP: bfd21140 ESP: bfd21140
[ 7.186491][ C0] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000202
[ 7.186496][ C0] BUG: scheduling while atomic: init/1/0x7fffffff
[ 7.186498][ C0] Modules linked in:
[ 7.186500][ C0] CPU: 0 PID: 1 Comm: init Tainted: G W 5.18.0-rc3-00017-g7d3fafb75102 #1
[ 7.186502][ C0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 7.186502][ C0] Call Trace:
[ 7.186503][ C0] ? show_stack+0x3d/0x45
[ 7.186506][ C0] dump_stack_lvl+0x34/0x44
[ 7.186508][ C0] dump_stack+0xd/0x10
[ 7.186511][ C0] __schedule_bug.cold+0x49/0x57
[ 7.186514][ C0] __schedule+0x642/0x780
[ 7.186516][ C0] ? wait_task_inactive+0xaa/0x1c0
[ 7.186519][ C0] schedule+0x49/0xc0
[ 7.186521][ C0] exit_to_user_mode_prepare+0x11a/0x180
[ 7.186525][ C0] syscall_exit_to_user_mode+0x1b/0x40
[ 7.186527][ C0] __do_fast_syscall_32+0x65/0x100
[ 7.186530][ C0] do_fast_syscall_32+0x29/0x80
[ 7.186532][ C0] do_SYSENTER_32+0x15/0x40
[ 7.186535][ C0] entry_SYSENTER_32+0x98/0xf1
[ 7.186538][ C0] EIP: 0xb7f7c589
[ 7.186539][ C0] Code: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d 76 00 58 b8 77 00 00 00 cd 80 90 8d 76
[ 7.186540][ C0] EAX: 00000000 EBX: 00000007 ECX: 00000056 EDX: 00000000
[ 7.186542][ C0] ESI: 00000000 EDI: b7ef9ff4 EBP: bfd211a8 ESP: bfd21140
[ 7.186543][ C0] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000202
[ 7.188748][ C0] ------------[ cut here ]------------
[ 7.188750][ C0] WARNING: CPU: 0 PID: 1 at arch/x86/kernel/fpu/signal.c:206 copy_fpstate_to_sigframe+0x1b8/0x2c0
[ 7.188758][ C0] Modules linked in:
[ 7.188761][ C0] CPU: 0 PID: 1 Comm: init Tainted: G W 5.18.0-rc3-00017-g7d3fafb75102 #1
[ 7.188763][ C0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 7.188764][ C0] EIP: copy_fpstate_to_sigframe+0x1b8/0x2c0
[ 7.188767][ C0] Code: 0b 8b 40 04 8b 55 b4 e8 ce 13 96 00 64 ff 0d d4 06 1e cf eb b3 8d 74 26 00 90 3b bb 80 10 00 00 75 86 eb aa 8d b6 00 00 00 00 <0f> 0b e9 92 fe ff ff 90 39 75 b0 0f 84 c7 00 00 00 8b 55 b0 8b 45
[ 7.188769][ C0] EAX: 000003b4 EBX: c114f600 ECX: c114e540 EDX: 00000000
[ 7.188771][ C0] ESI: bfd20e00 EDI: fffffd28 EBP: c1139ea0 ESP: c1139e4c
[ 7.188772][ C0] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010246
[ 7.188776][ C0] CR0: 80050033 CR2: b7e2bb60 CR3: 02f8e000 CR4: 000406f0
[ 7.188778][ C0] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 7.188779][ C0] DR6: fffe0ff0 DR7: 00000400
[ 7.188780][ C0] Call Trace:
[ 7.188783][ C0] ? kmem_cache_free+0x280/0x2c0
[ 7.188787][ C0] ? bigsmp_ioapic_phys_id_map+0x80/0x80
[ 7.188791][ C0] ? kvm_guest_apic_eoi_write+0x29/0x40
[ 7.188794][ C0] ? sysvec_reschedule_ipi+0x70/0x100
[ 7.188800][ C0] get_sigframe+0x161/0x280
[ 7.188807][ C0] __setup_frame+0x3c/0x200
[ 7.188810][ C0] ? sysvec_reboot+0x40/0x40
[ 7.188813][ C0] handle_signal+0xea/0x180
[ 7.188816][ C0] arch_do_signal_or_restart+0xaa/0xc0
[ 7.188819][ C0] exit_to_user_mode_prepare+0x135/0x180
[ 7.188823][ C0] syscall_exit_to_user_mode+0x1b/0x40
[ 7.188825][ C0] __do_fast_syscall_32+0x65/0x100
[ 7.188829][ C0] do_fast_syscall_32+0x29/0x80
[ 7.188831][ C0] do_SYSENTER_32+0x15/0x40
[ 7.188834][ C0] entry_SYSENTER_32+0x98/0xf1
[ 7.188839][ C0] EIP: 0xb7f7c589
[ 7.188840][ C0] Code: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d 76 00 58 b8 77 00 00 00 cd 80 90 8d 76
[ 7.188842][ C0] EAX: 00000000 EBX: 00004201 ECX: 00000056 EDX: 00000000
[ 7.188843][ C0] ESI: bfd211cc EDI: b7ef9ff4 EBP: bfd211e8 ESP: bfd21170
[ 7.188845][ C0] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000212
[ 7.188847][ C0] ---[ end trace 0000000000000000 ]---
To reproduce:
# build kernel
cd linux
cp config-5.18.0-rc3-00017-g7d3fafb75102 .config
make HOSTCC=gcc-11 CC=gcc-11 ARCH=i386 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-11 CC=gcc-11 ARCH=i386 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://01.org/lkp