Messages in this thread Patch in this message | | | Subject | Re: [PATCH v15 02/12] cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h> | From | David Woodhouse <> | Date | Mon, 20 Mar 2023 08:17:29 +0000 |
| |
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; [unhandled content-type:application/pkcs7-signature] | |