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

[Thomas Gleixner]

On Sat, 30 Dec 2017, Thomas Gleixner wrote:
> On Sat, 30 Dec 2017, Linus Torvalds wrote:
> > On Sat, Dec 30, 2017 at 10:20 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> > The __native_flush_tlb() function looks _very_ broken.
> > 
> > It does:
> > 
> >         invalidate_user_asid(this_cpu_read(cpu_tlbstate.loaded_mm_asid));
> >         /*
> >          * If current->mm == NULL then we borrow a mm which may change
> >          * during a task switch and therefore we must not be preempted
> >          * while we write CR3 back:
> >          */
> >         preempt_disable();
> >         native_write_cr3(__native_read_cr3());
> >         preempt_enable();
> > 
> > but why is that preempt-disabled region only around the cr3 write? The
> > invalidate_user_asid() logic seems to be very CPU-sensitive too.
> > 
> > And even if there is some reason why invalidate_user_asid() really can
> > do multiple different percpu accesses and it doesn't matter whether
> > the thread is bouncing around on different cpu's while it does it,
> > there doesn't seem any _reason_ not to just extend the preempt-disable
> > over the whole series.
> > 
> > It really looks strange how it does multiple reads (and then a final
> > write!) to percpu state, when the cpu can change in between.
> > 
> > So I'd suggest moving the preempt_disable() up to the top of that
> > function,
> 
> That preempt_disable()/enable() was added with:
> 
> commit 5cf0791da5c162ebc14b01eb01631cfa7ed4fa6e
> Author: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Date:   Fri Aug 5 15:37:39 2016 +0200
> 
>     x86/mm: Disable preemption during CR3 read+write
> 
> So we need to look at that scenario before removing it.

The UP distinction does not exist anymore since:

commit ce4a4e565f5264909a18c733b864c3f74467f69e
Author: Andy Lutomirski <luto@xxxxxxxxxx>
Date:   Sun May 28 10:00:14 2017 -0700

    x86/mm: Remove the UP asm/tlbflush.h code, always use the (formerly) SMP code


The proper fix for this would have been anyway to add the
preempt_disable()/enable() pair to

static inline void flush_tlb_mm_range(struct mm_struct *mm,
           unsigned long start, unsigned long end, unsigned long vmflag)
{
        if (mm == current->active_mm)
                __flush_tlb_up();
}

and not into __native_flush_tlb().

The common flush_tlb_mm_range() properly disables preemption, so we are
good to remove that.

Thanks,

	tglx