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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1712302142300.1899@nanos>
Date:   Sat, 30 Dec 2017 21:47:10 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
cc:     Dominik Brodowski <linux@...inikbrodowski.net>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        the arch/x86 maintainers <x86@...nel.org>
Subject: Re: x86/pti: smp_processor_id() called while preemptible in
 resume-from-sleep

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@...utronix.de> 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@...utronix.de>
> 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@...nel.org>
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