Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750978AbdL3U2H (ORCPT ); Sat, 30 Dec 2017 15:28:07 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:35194 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750828AbdL3U2G (ORCPT ); Sat, 30 Dec 2017 15:28:06 -0500 Date: Sat, 30 Dec 2017 21:28:03 +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: 1587 Lines: 44 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. Thanks, tglx