Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750996AbdL3UrU (ORCPT ); Sat, 30 Dec 2017 15:47:20 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:35208 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750888AbdL3UrT (ORCPT ); Sat, 30 Dec 2017 15:47:19 -0500 Date: Sat, 30 Dec 2017 21:47:10 +0100 (CET) From: Thomas Gleixner To: Linus Torvalds cc: Dominik Brodowski , Andy Lutomirski , Dave Hansen , LKML , the arch/x86 maintainers Subject: Re: x86/pti: smp_processor_id() called while preemptible in resume-from-sleep In-Reply-To: Message-ID: References: <20171230132927.GA2731@light.dominikbrodowski.net> <20171230153054.GA1604@light.dominikbrodowski.net> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2409 Lines: 69 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 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 > 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 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