Re: [PATCH 11/23] sched/scs: Initialize shadow stack on idle thread bringup, not shutdown

[Sami Tolvanen <samitolvanen@google.com>]

Hi Andy,

On Sat, Jan 8, 2022 at 8:44 AM Andy Lutomirski <luto@kernel.org> wrote:
>
> Starting with commit 63acd42c0d49 ("sched/scs: Reset the shadow stack when
> idle_task_exit"), the idle thread's shadow stack was reset from the idle
> task's context during CPU hot-unplug.  This was fragile: between resetting
> the shadow stack and actually stopping the idle task, the shadow stack
> did not match the actual call stack.
>
> Clean this up by resetting the idle task's SCS in bringup_cpu().
>
> init_idle() still does scs_task_reset() -- see the comments there.  I
> leave this to an SCS maintainer to untangle further.
>
> Cc: Woody Lin <woodylin@google.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  kernel/cpu.c        | 3 +++
>  kernel/sched/core.c | 9 ++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 192e43a87407..be16816bb87c 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -33,6 +33,7 @@
>  #include <linux/slab.h>
>  #include <linux/percpu-rwsem.h>
>  #include <linux/cpuset.h>
> +#include <linux/scs.h>
>
>  #include <trace/events/power.h>
>  #define CREATE_TRACE_POINTS
> @@ -587,6 +588,8 @@ static int bringup_cpu(unsigned int cpu)
>         struct task_struct *idle = idle_thread_get(cpu);
>         int ret;
>
> +       scs_task_reset(idle);
> +
>         /*
>          * Some architectures have to walk the irq descriptors to
>          * setup the vector space for the cpu which comes online.
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 917068b0a145..acd52a7d1349 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8621,7 +8621,15 @@ 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);
>
> +       /*
> +        * NB: This is called from sched_init() on the *current* idle thread.
> +        * This seems fragile if not actively incorrect.
> +        *
> +        * Initializing SCS for about-to-be-brought-up CPU idle threads
> +        * is in bringup_cpu(), but that does not cover the boot CPU.
> +        */
>         scs_task_reset(idle);
> +
>         kasan_unpoison_task_stack(idle);
>
>  #ifdef CONFIG_SMP
> @@ -8779,7 +8787,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 */
>  }

I believe Mark already fixed this one here:

https://lore.kernel.org/lkml/20211123114047.45918-1-mark.rutland@arm.com/

Sami