Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211012083521.973587-1-woodylin@google.com> <87zgrek1gl.mognet@arm.com>
In-Reply-To: <87zgrek1gl.mognet@arm.com>
From:   Woody Lin <woodylin@google.com>
Date:   Tue, 12 Oct 2021 18:35:48 +0800
Message-ID: <CAHn4Dec0Jyc30vWMLAXwQ-ge4eS5S26hxfMky-e4f-TTtFrbEQ@mail.gmail.com>
Subject: Re: [PATCH] sched/scs: Reset the shadow stack when idle_task_exit
To:     Valentin Schneider <valentin.schneider@arm.com>
Cc:     Ingo Molnar <mingo@redhat.com>, Ben Segall <bsegall@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 12, 2021 at 6:00 PM Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
> On 12/10/21 16:35, Woody Lin wrote:
> > There was a 'init_idle' that resets scs sp to base, but is removed by
> > f1a0a376ca0c. Without the resetting, the hot-plugging implemented by
> > cpu_psci_cpu_boot will use the previous scs sp as new base when starting
> > up a CPU core, so the usage on scs page is being stacked up until
> > overflow.
> >
> > This only happens on idle task since __cpu_up is using idle task as the
> > main thread to start up a CPU core, so the overflow can be fixed by
> > resetting scs sp to base in 'idle_task_exit'.
> >
>
> Looking at init_idle() for similar issues, it looks like we might also want
> to re-issue kasan_unpoison_task_stack() on the idle task upon hotplug.
>
> > Fixes: f1a0a376ca0c ("sched/core: Initialize the idle task with preemption disabled")
> > Signed-off-by: Woody Lin <woodylin@google.com>
> > ---
> >  kernel/sched/core.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 1bba4128a3e6..f21714ea3db8 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -8795,6 +8795,7 @@ 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 */
>
> So AIUI for SCS that works just fine - one thing I'm unclear on is how the
> following pops are going to work given the SP reset happens in the middle
> of a call stack, but AFAICT that was already the case before I messed about
> with init_idle(), so that must already be handled.

Hi Valentin,

Thanks for the question. The 'scs_task_reset' here resets only the
'.thread_info.scs_sp' of the task, so the register (on arm64 it's x18)
is still pointing to the same location for popping and storing call
frames. The register will be updated to '.thread_info.scs_sp' in
'__secondary_switched', which starts a new core and there is no popping
after the updating, so it won't introduce an underflow.

>
> I'm not familiar enough with KASAN to say whether that
> kasan_unpoison_task_stack() should rather happen upon hotplugging the CPU
> back (rather than on hot-unplug). If that is the case, then maybe somewhere
> around cpu_startup_entry() might work (and then you could bunch these two
> "needs to be re-run at init for the idle task" functions into a common
> helper).

unpoison looks more like an one-time thing to me; the idle tasks will
reuse the same stack pages until system resets, so I think we don't need
to re-unpoison that during hotplugging as long as it's unpoisoned in
'init_idle'.

>
> >  }
> >
> > --
> > 2.33.0.882.g93a45727a2-goog