On Sun, 2023-03-19 at 17:34 +0100, Borislav Petkov wrote: > Frankly, I don't think resetting shadow call stack and kasan state > belongs in a function which returns the idle thread. Even more so if you > have to add an @unpoison param which is false sometimes and sometimes > true, depending on where you call the function. > > I think you should have a helper > > tsk_reset_stacks(struct task_struct *tsk); > > or so which is called where @unpoison == true instead of having a getter > function do something unrelated too. Yeah, I see that but my primary concern was that I didn't want callers to be able to *forget* it, which is what happened the first time. I don't feel so bad about the getter function actually making the object *usable* as well as getting it. We don't have to remember to make a separate function call to unpoison a pointer newly returned from kmalloc either. I *think* the 'unpoison' arg is purely an optimisation anyway. Because those operations are, or at least *could* be, idempotent. It's just that it's a bit pointless doing them from the finish_cpu() call. But actually, I think we can have it both ways. There's an early call to idle_thread_get(cpu) in _cpu_up(), with the comment /* Let it fail before we try to bring the cpu up */ With an adjustment to the comment, we could do the unpoison and reset the shadow call stack there — in common code where no architecture can forget to do it — and still leave idle_thread_get() doing precisely just the 'get'. In _cpu_up() the call to idle_thread_get() only happens if the CPU in question is starting from CPUHP_OFFLINE but I think that's sufficient? diff --git a/kernel/cpu.c b/kernel/cpu.c index 6c0a92ca6bb5..43e0a77f21e8 100644 --- a/kernel/cpu.c +++ b/kernel/cpu.c @@ -591,12 +591,6 @@ static int bringup_cpu(unsigned int cpu) struct task_struct *idle = idle_thread_get(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. @@ -1383,6 +1377,12 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen, enum cpuhp_state target) ret = PTR_ERR(idle); goto out; } + + /* + * Reset stale stack state from the last time this CPU was online. + */ + scs_task_reset(idle); + kasan_unpoison_task_stack(idle); } cpuhp_tasks_frozen = tasks_frozen;
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
