From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932650AbcFODUy (ORCPT ); Tue, 14 Jun 2016 23:20:54 -0400 Received: from mail-pf0-f194.google.com ([209.85.192.194]:36186 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932354AbcFODUx convert rfc822-to-8bit (ORCPT ); Tue, 14 Jun 2016 23:20:53 -0400 Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: [PATCH] Linux VM workaround for Knights Landing A/D leak From: Nadav Amit In-Reply-To: <5760792D.90000@linux.intel.com> Date: Tue, 14 Jun 2016 20:20:46 -0700 Cc: Lukasz Anaczkowski , LKML , linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , Andi Kleen , "Kirill A. Shutemov" , Michal Hocko , Andrew Morton , "H. Peter Anvin" , harish.srinivasappa@intel.com, lukasz.odzioba@intel.com, Andy Lutomirski Content-Transfer-Encoding: 8BIT Message-Id: References: <1465919919-2093-1-git-send-email-lukasz.anaczkowski@intel.com> <7FB15233-B347-4A87-9506-A9E10D331292@gmail.com> <57603C61.5000408@linux.intel.com> <2471A3E8-FF69-4720-A3BF-BDC6094A6A70@gmail.com> <5760792D.90000@linux.intel.com> To: Dave Hansen X-Mailer: Apple Mail (2.3124) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Dave Hansen wrote: > On 06/14/2016 01:16 PM, Nadav Amit wrote: >> Dave Hansen wrote: >> >>> On 06/14/2016 09:47 AM, Nadav Amit wrote: >>>> Lukasz Anaczkowski wrote: >>>> >>>>>> From: Andi Kleen >>>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep) >>>>>> +{ >>>> Here there should be a call to smp_mb__after_atomic() to synchronize with >>>> switch_mm. I submitted a similar patch, which is still pending (hint). >>>> >>>>>> + if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) { >>>>>> + trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL); >>>>>> + flush_tlb_others(mm_cpumask(mm), mm, addr, >>>>>> + addr + PAGE_SIZE); >>>>>> + mb(); >>>>>> + set_pte(ptep, __pte(0)); >>>>>> + } >>>>>> +} >>> >>> Shouldn't that barrier be incorporated in the TLB flush code itself and >>> not every single caller (like this code is)? >>> >>> It is insane to require individual TLB flushers to be concerned with the >>> barriers. >> >> IMHO it is best to use existing flushing interfaces instead of creating >> new ones. > > Yeah, or make these things a _little_ harder to get wrong. That little > snippet above isn't so crazy that we should be depending on open-coded > barriers to get it right. > > Should we just add a barrier to mm_cpumask() itself? That should stop > the race. Or maybe we need a new primitive like: > > /* > * Call this if a full barrier has been executed since the last > * pagetable modification operation. > */ > static int __other_cpus_need_tlb_flush(struct mm_struct *mm) > { > /* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */ > return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < > nr_cpu_ids; > } > > > static int other_cpus_need_tlb_flush(struct mm_struct *mm) > { > /* > * Synchronizes with switch_mm. Makes sure that we do not > * observe a bit having been cleared in mm_cpumask() before > * the other processor has seen our pagetable update. See > * switch_mm(). > */ > smp_mb__after_atomic(); > > return __other_cpus_need_tlb_flush(mm) > } > > We should be able to deploy other_cpus_need_tlb_flush() in most of the > cases where we are doing "cpumask_any_but(mm_cpumask(mm), > smp_processor_id()) < nr_cpu_ids". > > Right? This approach may work, but I doubt other_cpus_need_tlb_flush() would be needed by anyone, excluding this "hacky" workaround. There are already five interfaces for invalidation of: a single page, a userspace range, a whole task, a kernel range, and full flush including kernel (global) entries. > >> In theory, fix_pte_leak could have used flush_tlb_page. But the problem >> is that flush_tlb_page requires the vm_area_struct as an argument, which >> ptep_get_and_clear (and others) do not have. > > That, and we do not want/need to flush the _current_ processor's TLB. > flush_tlb_page() would have done that unnecessarily. That's not the end > of the world here, but it is a downside. Oops, I missed the fact a local flush is not needed in this case. Nadav