To hot unplug a CPU, the idle task on that CPU calls a few layers of C
code before finally leaving the kernel. When KASAN is in use, poisoned
shadow is left around for each of the active stack frames, and when
shadow call stacks are in use. When shadow call stacks are in use the
task's SCS SP is left pointing at an arbitrary point within the task's
shadow call stack.
When an offlines CPU is hotlpugged back into the kernel, this stale
state can adversely affect the newly onlined CPU. Stale KASAN shadow can
alias new stackframes and result in bogus KASAN warnings. A stale SCS SP
is effectively a memory leak, and prevents a portion of the shadow call
stack being used. Across a number of hotplug cycles the task's entire
shadow call stack can become unusable.
We previously fixed the KASAN issue in commit:
e1b77c92981a5222 ("sched/kasan: remove stale KASAN poison after hotplug")
In commit:
f1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
... we broke both KASAN and SCS, with SCS being fixed up in commit:
63acd42c0d4942f7 ("sched/scs: Reset the shadow stack when idle_task_exit")
... but as this runs in the context of the idle task being offlines it's
potentially fragile.
Fix both of these consistently and more robustly by resetting the SCS SP
and KASAN shadow immediately before we online a CPU. This ensures the
idle task always has a consistent state, and removes the need to do so
when initializing an idle task or when unplugging an idle task.
I've tested this with both GCC and clang, with reelvant options enabled,
offlining and online CPUs with:
| while true; do
| for C in /sys/devices/system/cpu/cpu*/online; do
| echo 0 > $C;
| echo 1 > $C;
| done
| done
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/linux-arm-kernel/YY9ECKyPtDbD9q8q@qian-HP-Z2-SFF-G5-Workstation/
Fixes: 1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
Reported-by: Qian Cai <[email protected]>
Signed-off-by: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Woody Lin <[email protected]>
---
kernel/cpu.c | 7 +++++++
kernel/sched/core.c | 4 ----
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 192e43a87407..407a2568f35e 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -31,6 +31,7 @@
#include <linux/smpboot.h>
#include <linux/relay.h>
#include <linux/slab.h>
+#include <linux/scs.h>
#include <linux/percpu-rwsem.h>
#include <linux/cpuset.h>
@@ -588,6 +589,12 @@ static int bringup_cpu(unsigned int cpu)
int ret;
/*
+ * Reset stale stack state from the last time this CPU was online.
+ */
+ scs_task_reset(idle);
+ kasan_unpoison_task_stack(idle);
+
+ /*
* Some architectures have to walk the irq descriptors to
* setup the vector space for the cpu which comes online.
* Prevent irq alloc/free across the bringup.
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3c9b0fda64ac..76f9deeaa942 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8619,9 +8619,6 @@ void __init init_idle(struct task_struct *idle, int cpu)
idle->flags |= PF_IDLE | PF_KTHREAD | PF_NO_SETAFFINITY;
kthread_set_per_cpu(idle, cpu);
- scs_task_reset(idle);
- kasan_unpoison_task_stack(idle);
-
#ifdef CONFIG_SMP
/*
* It's possible that init_idle() gets called multiple times on a task,
@@ -8777,7 +8774,6 @@ void idle_task_exit(void)
finish_arch_post_lock_switch();
}
- scs_task_reset(current);
/* finish_cpu(), as ran on the BP, will clean up the active_mm state */
}
--
2.11.0
Hi Mark,
Thanks for tackling this and glueing the pieces back together. LGTM, though
I couldn't stop myself from playing changelog police - I also have a
question/comment wrt the BP.
On 15/11/21 11:33, Mark Rutland wrote:
> To hot unplug a CPU, the idle task on that CPU calls a few layers of C
> code before finally leaving the kernel. When KASAN is in use, poisoned
> shadow is left around for each of the active stack frames, and when
> shadow call stacks are in use. When shadow call stacks are in use the
> task's SCS SP is left pointing at an arbitrary point within the task's
> shadow call stack.
>
> When an offlines CPU is hotlpugged back into the kernel, this stale
^^^^^^^^ ^^^^^^^^^^
offlined? hotplugged
> state can adversely affect the newly onlined CPU. Stale KASAN shadow can
> alias new stackframes and result in bogus KASAN warnings. A stale SCS SP
> is effectively a memory leak, and prevents a portion of the shadow call
> stack being used. Across a number of hotplug cycles the task's entire
> shadow call stack can become unusable.
>
> We previously fixed the KASAN issue in commit:
>
> e1b77c92981a5222 ("sched/kasan: remove stale KASAN poison after hotplug")
>
> In commit:
>
> f1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
>
> ... we broke both KASAN and SCS, with SCS being fixed up in commit:
>
> 63acd42c0d4942f7 ("sched/scs: Reset the shadow stack when idle_task_exit")
>
> ... but as this runs in the context of the idle task being offlines it's
^^^^^^^^
offlined
> potentially fragile.
>
> Fix both of these consistently and more robustly by resetting the SCS SP
> and KASAN shadow immediately before we online a CPU. This ensures the
> idle task always has a consistent state, and removes the need to do so
> when initializing an idle task or when unplugging an idle task.
>
> I've tested this with both GCC and clang, with reelvant options enabled,
^^^^^^^^
relevant
> offlining and online CPUs with:
^^^^^^
onlining
>
> | while true; do
> | for C in /sys/devices/system/cpu/cpu*/online; do
> | echo 0 > $C;
> | echo 1 > $C;
> | done
> | done
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Link: https://lore.kernel.org/linux-arm-kernel/YY9ECKyPtDbD9q8q@qian-HP-Z2-SFF-G5-Workstation/
> Fixes: 1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
> Reported-by: Qian Cai <[email protected]>
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Woody Lin <[email protected]>
Reviewed-by: Valentin Schneider <[email protected]>
> ---
> kernel/cpu.c | 7 +++++++
> kernel/sched/core.c | 4 ----
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 192e43a87407..407a2568f35e 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -31,6 +31,7 @@
> #include <linux/smpboot.h>
> #include <linux/relay.h>
> #include <linux/slab.h>
> +#include <linux/scs.h>
> #include <linux/percpu-rwsem.h>
> #include <linux/cpuset.h>
>
> @@ -588,6 +589,12 @@ static int bringup_cpu(unsigned int cpu)
> int ret;
>
> /*
> + * Reset stale stack state from the last time this CPU was online.
> + */
> + scs_task_reset(idle);
> + kasan_unpoison_task_stack(idle);
> +
> + /*
> * Some architectures have to walk the irq descriptors to
> * setup the vector space for the cpu which comes online.
> * Prevent irq alloc/free across the bringup.
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3c9b0fda64ac..76f9deeaa942 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8619,9 +8619,6 @@ void __init init_idle(struct task_struct *idle, int cpu)
> idle->flags |= PF_IDLE | PF_KTHREAD | PF_NO_SETAFFINITY;
> kthread_set_per_cpu(idle, cpu);
>
> - scs_task_reset(idle);
> - kasan_unpoison_task_stack(idle);
> -
So those are no longer invoked for the BP during bootup (via sched_init());
that looks OK for KASAN per:
e1b77c92981a ("sched/kasan: remove stale KASAN poison after hotplug")
I didn't find any explicit commit for SCS but from the looks of
arm64/include/asm/thread_info.h we seem to be initializing things
correctly, so IIUC the removed hunk wasn't actually necessary for the BP's
first boot.
> #ifdef CONFIG_SMP
> /*
> * It's possible that init_idle() gets called multiple times on a task,
> @@ -8777,7 +8774,6 @@ void idle_task_exit(void)
> finish_arch_post_lock_switch();
> }
>
> - scs_task_reset(current);
> /* finish_cpu(), as ran on the BP, will clean up the active_mm state */
> }
>
> --
> 2.11.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Nov 15, 2021 at 12:16:14PM +0000, Valentin Schneider wrote:
>
> Hi Mark,
>
> Thanks for tackling this and glueing the pieces back together. LGTM, though
> I couldn't stop myself from playing changelog police - I also have a
> question/comment wrt the BP.
I'll go fix the various typos for v2; replies to the more substantial comments
below.
> On 15/11/21 11:33, Mark Rutland wrote:
> > To hot unplug a CPU, the idle task on that CPU calls a few layers of C
> > code before finally leaving the kernel. When KASAN is in use, poisoned
> > shadow is left around for each of the active stack frames, and when
> > shadow call stacks are in use. When shadow call stacks are in use the
> > task's SCS SP is left pointing at an arbitrary point within the task's
> > shadow call stack.
> > Fix both of these consistently and more robustly by resetting the SCS SP
> > and KASAN shadow immediately before we online a CPU.
> Reviewed-by: Valentin Schneider <[email protected]>
Thanks!
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 3c9b0fda64ac..76f9deeaa942 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -8619,9 +8619,6 @@ void __init init_idle(struct task_struct *idle, int cpu)
> > idle->flags |= PF_IDLE | PF_KTHREAD | PF_NO_SETAFFINITY;
> > kthread_set_per_cpu(idle, cpu);
> >
> > - scs_task_reset(idle);
> > - kasan_unpoison_task_stack(idle);
> > -
>
> So those are no longer invoked for the BP during bootup (via sched_init());
> that looks OK for KASAN per:
>
> e1b77c92981a ("sched/kasan: remove stale KASAN poison after hotplug")
>
> I didn't find any explicit commit for SCS but from the looks of
> arm64/include/asm/thread_info.h we seem to be initializing things
> correctly, so IIUC the removed hunk wasn't actually necessary for the BP's
> first boot.
Correct. For the init task:
* The KASAN shadow starts out empty, and there's no poison to remove.
* The saved SCS SP is initialized statically as part of INIT_THREAD_INFO(), via
INIT_SCS().
... so that requires no special care.
For every task created thereafter (including idle threads):
* dup_task_struct() allocates the new task's stack via
alloc_thread_stack_node(), which either explicitly removes KASAN poison from
the shadow of a cached stack, or acquires a stack via __vmalloc_node_range(),
whose shadow starts out empty.
* dup_task_struct() calls scs_prepare() which allocates the task's shadow stack
and initializes the SCS SP for the task.
The idle threads get created via fork_idle(), which calls copy_process() (and
therefore dup_task_struct) to allocate the idle thread, then calls init_idle()
on the result.
Thanks
Mark.
On Mon, Nov 15, 2021 at 02:09:27PM +0000, Mark Rutland wrote:
> On Mon, Nov 15, 2021 at 12:16:14PM +0000, Valentin Schneider wrote:
> >
> > Hi Mark,
> >
> > Thanks for tackling this and glueing the pieces back together. LGTM, though
> > I couldn't stop myself from playing changelog police - I also have a
> > question/comment wrt the BP.
>
> I'll go fix the various typos for v2; replies to the more substantial comments
> below.
FWIW, I pushed out a version with an updated commit message to:
https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=sched/hotplug-stack-reset
git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git sched/hotplug-stack-reset
... and I'll wait a little bit before posting that in case there's
any further comment on the patch itself.
Thanks,
Mark.
On Mon, Nov 15, 2021 at 11:33:10AM +0000, Mark Rutland wrote:
> To hot unplug a CPU, the idle task on that CPU calls a few layers of C
> code before finally leaving the kernel. When KASAN is in use, poisoned
> shadow is left around for each of the active stack frames, and when
> shadow call stacks are in use. When shadow call stacks are in use the
> task's SCS SP is left pointing at an arbitrary point within the task's
> shadow call stack.
>
> When an offlines CPU is hotlpugged back into the kernel, this stale
> state can adversely affect the newly onlined CPU. Stale KASAN shadow can
> alias new stackframes and result in bogus KASAN warnings. A stale SCS SP
> is effectively a memory leak, and prevents a portion of the shadow call
> stack being used. Across a number of hotplug cycles the task's entire
> shadow call stack can become unusable.
>
> We previously fixed the KASAN issue in commit:
>
> e1b77c92981a5222 ("sched/kasan: remove stale KASAN poison after hotplug")
>
> In commit:
>
> f1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
>
> ... we broke both KASAN and SCS, with SCS being fixed up in commit:
>
> 63acd42c0d4942f7 ("sched/scs: Reset the shadow stack when idle_task_exit")
>
> ... but as this runs in the context of the idle task being offlines it's
> potentially fragile.
>
> Fix both of these consistently and more robustly by resetting the SCS SP
> and KASAN shadow immediately before we online a CPU. This ensures the
> idle task always has a consistent state, and removes the need to do so
> when initializing an idle task or when unplugging an idle task.
>
> I've tested this with both GCC and clang, with reelvant options enabled,
> offlining and online CPUs with:
>
> | while true; do
> | for C in /sys/devices/system/cpu/cpu*/online; do
> | echo 0 > $C;
> | echo 1 > $C;
> | done
> | done
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Link: https://lore.kernel.org/linux-arm-kernel/YY9ECKyPtDbD9q8q@qian-HP-Z2-SFF-G5-Workstation/
> Fixes: 1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
> Reported-by: Qian Cai <[email protected]>
> Signed-off-by: Mark Rutland <[email protected]>
Thanks for fixing this quickly, Mark. Triggering an user-after-free in
user namespace but don't think it is related. I'll investigate that
first since it is blocking the rest of regression testing.
On Tue, Nov 16, 2021 at 11:31:40AM -0500, Qian Cai wrote:
> On Mon, Nov 15, 2021 at 11:33:10AM +0000, Mark Rutland wrote:
> > To hot unplug a CPU, the idle task on that CPU calls a few layers of C
> > code before finally leaving the kernel. When KASAN is in use, poisoned
> > shadow is left around for each of the active stack frames, and when
> > shadow call stacks are in use. When shadow call stacks are in use the
> > task's SCS SP is left pointing at an arbitrary point within the task's
> > shadow call stack.
> >
> > When an offlines CPU is hotlpugged back into the kernel, this stale
> > state can adversely affect the newly onlined CPU. Stale KASAN shadow can
> > alias new stackframes and result in bogus KASAN warnings. A stale SCS SP
> > is effectively a memory leak, and prevents a portion of the shadow call
> > stack being used. Across a number of hotplug cycles the task's entire
> > shadow call stack can become unusable.
> >
> > We previously fixed the KASAN issue in commit:
> >
> > e1b77c92981a5222 ("sched/kasan: remove stale KASAN poison after hotplug")
> >
> > In commit:
> >
> > f1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
> >
> > ... we broke both KASAN and SCS, with SCS being fixed up in commit:
> >
> > 63acd42c0d4942f7 ("sched/scs: Reset the shadow stack when idle_task_exit")
> >
> > ... but as this runs in the context of the idle task being offlines it's
> > potentially fragile.
> >
> > Fix both of these consistently and more robustly by resetting the SCS SP
> > and KASAN shadow immediately before we online a CPU. This ensures the
> > idle task always has a consistent state, and removes the need to do so
> > when initializing an idle task or when unplugging an idle task.
> >
> > I've tested this with both GCC and clang, with reelvant options enabled,
> > offlining and online CPUs with:
> >
> > | while true; do
> > | for C in /sys/devices/system/cpu/cpu*/online; do
> > | echo 0 > $C;
> > | echo 1 > $C;
> > | done
> > | done
> >
> > Link: https://lore.kernel.org/lkml/[email protected]/
> > Link: https://lore.kernel.org/linux-arm-kernel/YY9ECKyPtDbD9q8q@qian-HP-Z2-SFF-G5-Workstation/
> > Fixes: 1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
> > Reported-by: Qian Cai <[email protected]>
> > Signed-off-by: Mark Rutland <[email protected]>
>
> Thanks for fixing this quickly, Mark. Triggering an user-after-free in
> user namespace but don't think it is related. I'll investigate that
> first since it is blocking the rest of regression testing.
Cool; are you happy to provide a Tested-by tag for this patch? :)
Thanks,
Mark.
On Wed, Nov 17, 2021 at 11:52:34AM +0000, Mark Rutland wrote:
> > Thanks for fixing this quickly, Mark. Triggering an user-after-free in
> > user namespace but don't think it is related. I'll investigate that
> > first since it is blocking the rest of regression testing.
>
> Cool; are you happy to provide a Tested-by tag for this patch? :)
Sure, the testing is running good so far.
Tested-by: Qian Cai <[email protected]>
On Mon, 15 Nov 2021 at 11:33:10 +0000, Mark Rutland wrote:
>
> To hot unplug a CPU, the idle task on that CPU calls a few layers of C
> code before finally leaving the kernel. When KASAN is in use, poisoned
> shadow is left around for each of the active stack frames, and when
> shadow call stacks are in use. When shadow call stacks are in use the
> task's SCS SP is left pointing at an arbitrary point within the task's
> shadow call stack.
>
> When an offlines CPU is hotlpugged back into the kernel, this stale
> state can adversely affect the newly onlined CPU. Stale KASAN shadow can
> alias new stackframes and result in bogus KASAN warnings. A stale SCS SP
> is effectively a memory leak, and prevents a portion of the shadow call
> stack being used. Across a number of hotplug cycles the task's entire
> shadow call stack can become unusable.
>
> We previously fixed the KASAN issue in commit:
>
> e1b77c92981a5222 ("sched/kasan: remove stale KASAN poison after hotplug")
>
> In commit:
>
> f1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
>
> ... we broke both KASAN and SCS, with SCS being fixed up in commit:
>
> 63acd42c0d4942f7 ("sched/scs: Reset the shadow stack when idle_task_exit")
>
> ... but as this runs in the context of the idle task being offlines it's
> potentially fragile.
>
> Fix both of these consistently and more robustly by resetting the SCS SP
> and KASAN shadow immediately before we online a CPU. This ensures the
> idle task always has a consistent state, and removes the need to do so
> when initializing an idle task or when unplugging an idle task.
>
> I've tested this with both GCC and clang, with reelvant options enabled,
> offlining and online CPUs with:
>
> > while true; do
> > for C in /sys/devices/system/cpu/cpu*/online; do
> > echo 0 > $C;
> > echo 1 > $C;
> > done
> > done
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Link: https://lore.kernel.org/linux-arm-kernel/YY9ECKyPtDbD9q8q@qian-HP-Z2-SFF-G5-Workstation/
> Fixes: 1a0a376ca0c4ef1 ("sched/core: Initialize the idle task with preemption disabled")
> Reported-by: Qian Cai <[email protected]>
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Woody Lin <[email protected]>
>
> ---
> kernel/cpu.c | 7 +++++++
> kernel/sched/core.c | 4 ----
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 192e43a87407..407a2568f35e 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -31,6 +31,7 @@
> #include <linux/smpboot.h>
> #include <linux/relay.h>
> #include <linux/slab.h>
> +#include <linux/scs.h>
> #include <linux/percpu-rwsem.h>
> #include <linux/cpuset.h>
>
> @@ -588,6 +589,12 @@ static int bringup_cpu(unsigned int cpu)
> int ret;
>
> /*
> + * Reset stale stack state from the last time this CPU was
> online.
> + */
> + scs_task_reset(idle);
> + kasan_unpoison_task_stack(idle);
Hm, in the !CONFIG_GENERIC_SMP_IDLE_THREAD case, idle_thread_get() will
have returned NULL. Won't these then crash?
Admittedly that seems to be *only* for UM, as all other architectures
with SMP seem to set CONFIG_GENERIC_SMP_IDLE_THREAD.
cf.
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
>Hm, in the !CONFIG_GENERIC_SMP_IDLE_THREAD case, idle_thread_get() will
>have returned NULL. Won't these then crash?
>
>Admittedly that seems to be *only* for UM, as all other architectures
>with SMP seem to set CONFIG_GENERIC_SMP_IDLE_THREAD.
UM doesn't even have SMP, so maybe CONFIG_GENERIC_SMP_IDLE_THREAD can just be removed?
johannes
--
Sent from my phone.