Re: x86/pti: smp_processor_id() called while preemptible in resume-from-sleep

[Thomas Gleixner]

On Sat, 30 Dec 2017, Dave Hansen wrote:
> On 12/30/2017 07:30 AM, Dominik Brodowski wrote:
> > 
> > native_cpu_up+0x447/0xa30:
> > kern_pcid at arch/x86/include/asm/tlbflush.h:105
> >  (inlined by) invalidate_user_asid at arch/x86/include/asm/tlbflush.h:342
> >  (inlined by) __native_flush_tlb at arch/x86/include/asm/tlbflush.h:351
> >  (inlined by) smpboot_restore_warm_reset_vector at arch/x86/kernel/smpboot.c:146
> >  (inlined by) do_boot_cpu at arch/x86/kernel/smpboot.c:1022
> >  (inlined by) native_cpu_up at arch/x86/kernel/smpboot.c:1070
> 
> This appears to be this path:
> 
> > static inline void smpboot_restore_warm_reset_vector(void)
> > {
> >         unsigned long flags;
> > 
> >         /*
> >          * Install writable page 0 entry to set BIOS data area.
> >          */
> >         local_flush_tlb();
> 
> The PTI code is now tracking when a given ASID needs to get flushed in a
> per-cpu variable, and we use smp_processor_id() in local_flush_tlb() to
> do that tracking.  That's the *proximate* cause of the new warning.  I
> think it's actually a quite valid warning that's catching something
> questionable.
> 
> I'm limited here by not knowing how the warm reset vector actually
> works, though.  I don't know why we TLB flush at all, much less why we
> do it after CMOS_WRITE() in the "setup" path but _before_ CMOS_WRITE()
> in the "restore" one.  Where do we actually "Install writable page 0
> entry to set BIOS data area"?  Shouldn't we just be flushing _there_?
> 
> But, even _doing_ a TLB flush with preempt enabled and interrupts on
> seems wrong to me.  It just fundamentally doesn't mean anything because
> it can theoretically run anywhere and flush *any* TLB.  There might be
> some other implicit preempt-thwarting going on here, but I can't find it.
> 
> The naive fix here is to just preempt_dis/enable() over the area doing
> the flush and the writes to the TRAMPOLINE_* area.  That'll definitely
> shut up the warnings.

Well, yes, but it makes no sense at all.