Messages in this thread | | | Subject | Re: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online | From | Andrey Ryabinin <> | Date | Wed, 2 Mar 2016 18:27:49 +0300 |
| |
On 03/02/2016 05:50 PM, Mark Rutland wrote: > Hi, > > On Wed, Mar 02, 2016 at 04:51:59PM +0300, Andrey Ryabinin wrote: >> KASAN poisons stack redzones on function's entrance and unpoisons prior >> return. So when cpu goes offline the stack of idle task left poisoned. >> When cpu goes back online it re-enters the kernel via another path and >> starts using idle task's stack. Hence it's possible to hit stale poison >> values which results in false-positive KASAN splats. >> >> This patch registers cpu hotplug notifier which unpoisons idle task's >> stack prior to onlining cpu. > > Sorry, I failed to spot this before sending my series just now. > > FWIW, I have no strong feelings either way as to how we hook up the > stack shadow clearing in the hotplug case. >
In fact, I'm also don't have strong opinion on this.
Ingo, Peter, what's your preference? These patches or http://lkml.kernel.org/g/<1456928778-22491-3-git-send-email-mark.rutland@arm.com> ?
> It would be good if we could organise to share the infrastructure for > idle, though. > > Otherwise, I have a couple of comments below. > >> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> >> --- >> include/linux/sched.h | 6 ++++++ >> kernel/smpboot.h | 2 -- >> mm/kasan/kasan.c | 33 +++++++++++++++++++++++++++------ >> 3 files changed, 33 insertions(+), 8 deletions(-) >> >> diff --git a/include/linux/sched.h b/include/linux/sched.h >> index a10494a..18e526d 100644 >> --- a/include/linux/sched.h >> +++ b/include/linux/sched.h >> @@ -337,6 +337,12 @@ extern asmlinkage void schedule_tail(struct task_struct *prev); >> extern void init_idle(struct task_struct *idle, int cpu); >> extern void init_idle_bootup_task(struct task_struct *idle); >> >> +#ifdef CONFIG_GENERIC_SMP_IDLE_THREAD >> +extern struct task_struct *idle_thread_get(unsigned int cpu); >> +#else >> +static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; } >> +#endif >> + >> extern cpumask_var_t cpu_isolated_map; >> >> extern int runqueue_is_locked(int cpu); >> diff --git a/kernel/smpboot.h b/kernel/smpboot.h >> index 72415a0..eebf9ec 100644 >> --- a/kernel/smpboot.h >> +++ b/kernel/smpboot.h >> @@ -4,11 +4,9 @@ >> struct task_struct; >> >> #ifdef CONFIG_GENERIC_SMP_IDLE_THREAD >> -struct task_struct *idle_thread_get(unsigned int cpu); >> void idle_thread_set_boot_cpu(void); >> void idle_threads_init(void); >> #else >> -static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; } >> static inline void idle_thread_set_boot_cpu(void) { } >> static inline void idle_threads_init(void) { } >> #endif > > Is all the above necessary? > > Surely we can just include <linux/smpboot.h> in mm/kasan/kasan.c? >
It is necessary. kernel/smpboot.h != include/linux/smpboot.h
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c >> index bc0a8d8..c4ffd82 100644 >> --- a/mm/kasan/kasan.c >> +++ b/mm/kasan/kasan.c >> @@ -16,6 +16,7 @@ >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> #define DISABLE_BRANCH_PROFILING >> >> +#include <linux/cpu.h> >> #include <linux/export.h> >> #include <linux/init.h> >> #include <linux/kernel.h> >> @@ -537,16 +538,36 @@ static int kasan_mem_notifier(struct notifier_block *nb, >> { >> return (action == MEM_GOING_ONLINE) ? NOTIFY_BAD : NOTIFY_OK; >> } >> +#endif >> + >> +static int kasan_cpu_callback(struct notifier_block *nfb, >> + unsigned long action, void *hcpu) >> +{ >> + unsigned int cpu = (unsigned long)hcpu; >> + >> + if ((action == CPU_UP_PREPARE) || (action == CPU_UP_PREPARE_FROZEN)) { >> + struct task_struct *tidle = idle_thread_get(cpu); >> + kasan_unpoison_shadow(task_stack_page(tidle), THREAD_SIZE); > > We never expect the stack to hit the end of the thread_info, so we can > start at task_stack_page(tidle) + 1, and avoid the shadow for > sizeof(struct thread_info). >
I wouldn't bother, it's simpler to unpoison all. Size of struct thread_info is 32-bytes. That's 4-bytes of shadow. I don't think it matters whether you do memset of 2048 or 2044 bytes.
> Do we do any poisoning of the thread_info structure in the thread_union?
No, why would we poison it? It's absolutely valid memory and available for access.
> If so, we'd be erroneously clearing it here. > > Thanks, > Mark. >
| |