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;