On 05.10.2020 14:23, Andrew Cooper wrote:
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool
> remove)
> if ( IS_ENABLED(CONFIG_PV32) )
> FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
>
> + if ( stack_base[cpu] )
> + memguard_unguard_stack(stack_base[cpu]);
> +
> if ( remove )
> {
> FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
> FREE_XENHEAP_PAGE(idt_tables[cpu]);
>
> if ( stack_base[cpu] )
> - {
> - memguard_unguard_stack(stack_base[cpu]);
> FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
> - }
> }
> }
In my initial reply to Marek's report I did suggest putting the fix
in the alloc path in order to keep the pages "guarded" while the CPU
is parked, as the CPU during that period may still access at least
some of the stacks (e.g. when sending it an NMI IPI to enter a deeper
C state).
Otherwise, if the fix really was to remain here, the if() could now
also be dropped. And in this case I'd also suggest adding the new
piece of code a few lines earlier, so that all the
FREE_XENHEAP_PAGE() stay close together.
Jan
