From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Zijlstra Subject: Re: [PATCH v5 00/11] Introduces new count-based method for tracking lockless pagetable walks Date: Fri, 4 Oct 2019 13:42:36 +0200 Message-ID: <20191004114236.GD19463@hirez.programming.kicks-ass.net> References: <20191003013325.2614-1-leonardo@linux.ibm.com> <20191003072952.GN4536@hirez.programming.kicks-ass.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Leonardo Bras Cc: Song Liu , Michal Hocko , "Dmitry V. Levin" , Keith Busch , linux-mm@kvack.org, Paul Mackerras , Christoph Lameter , Ira Weiny , Dan Williams , Elena Reshetova , linux-arch@vger.kernel.org, Santosh Sivaraj , Davidlohr Bueso , "Aneesh Kumar K.V" , Bartlomiej Zolnierkiewicz , Mike Rapoport , Jason Gunthorpe , Vlastimil Babka , Mahesh Salgaonkar , Andrey Ryabinin , Alexey List-Id: linux-arch.vger.kernel.org On Thu, Oct 03, 2019 at 05:36:31PM -0300, Leonardo Bras wrote: > > Also, I'm not sure I understand things properly. > >=20 > > So serialize_against_pte_lookup() wants to wait for all currently > > out-standing __find_linux_pte() instances (which are very similar to > > gup_fast). > >=20 > > It seems to want to do this before flushing the THP TLB for some reason; > > why? Should not THP observe the normal page table freeing rules which > > includes a RCU-like grace period like this already. > >=20 > > Why is THP special here? This doesn't seem adequately explained. >=20 > "It's necessary to monitor lockless pagetable walks, in order to avoid > doing THP splitting/collapsing during them." >=20 > If a there is a THP split/collapse during the lockless pagetable walk, > the returned ptep can be a pointing to an invalid pte.=20 So the whole premise of lockless page-table walks (gup_fast) is that it can work on in-flux page-tables. Specifically gup_fast() never returns PTEs, only struct page *, and only if it can increment the page refcount. In order to enable this, page-table pages are RCU(-like) freed, such that even if we access page-tables that have (concurrently) been unlinked, we'll not UaF (see asm-generic/tlb.h, the comment at HAVE_RCU_TABLE_FREE). IOW, the worst case if not getting a struct page *. I really don't see how THP splitting/collapsing is special here, either we see the PMD and find a struct page * or we see a PTE and find the same struct page * (compound page head). The only thing that needs to be guaranteed is that both PTEs and PMD page-tables are valid. Is this not so? > To avoid that, the pmd is updated, then serialize_against_pte_lookup is > ran. Serialize runs a do_nothing in all cpu in cpu_mask.=20 >=20 > So, after all cpus finish running do_nothing(), there is a guarantee > that if there is any 'lockless pagetable walk' it is running on top of > a updated version of this pmd, and so, collapsing/splitting THP is > safe. But why would it matter?! It would find the same struct page * through either version of the page-tables. *confused* > > Also, specifically to munmap(), this seems entirely superfluous, > > munmap() uses the normal page-table freeing code and should be entirely > > fine without additional waiting. >=20 > To be honest, I remember it being needed in munmap case, but I really > don't remember the details. I will take a deeper look and come back > with this answer.=20 munmap does normal mmu_gather page-table teardown, the THP PMD should be RCU-like freed just like any other PMD. Which should be perfectly safe vs lockless page-table walks. If you can find anything there that isn't right, please explain that in detail and we'll need to look hard at fixing _that_. > > Furthermore, Power never accurately tracks mm_cpumask(), so using that > > makes the whole thing more expensive than it needs to be. Also, I > > suppose that is buggered vs file backed THP. >=20 > That accuracy of mm_cpumask is above my knowledge right now. =3D) Basically PowerPC only ever sets bits in there, unlike x86 that also clears bits (at expense, but it's worth it for us). From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from bombadil.infradead.org ([198.137.202.133]:57350 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725826AbfJDLnP (ORCPT ); Fri, 4 Oct 2019 07:43:15 -0400 Date: Fri, 4 Oct 2019 13:42:36 +0200 From: Peter Zijlstra Subject: Re: [PATCH v5 00/11] Introduces new count-based method for tracking lockless pagetable walks Message-ID: <20191004114236.GD19463@hirez.programming.kicks-ass.net> References: <20191003013325.2614-1-leonardo@linux.ibm.com> <20191003072952.GN4536@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Leonardo Bras Cc: Song Liu , Michal Hocko , "Dmitry V. Levin" , Keith Busch , linux-mm@kvack.org, Paul Mackerras , Christoph Lameter , Ira Weiny , Dan Williams , Elena Reshetova , linux-arch@vger.kernel.org, Santosh Sivaraj , Davidlohr Bueso , "Aneesh Kumar K.V" , Bartlomiej Zolnierkiewicz , Mike Rapoport , Jason Gunthorpe , Vlastimil Babka , Mahesh Salgaonkar , Andrey Ryabinin , Alexey Dobriyan , Ingo Molnar , Andrea Arcangeli , Ralph Campbell , Arnd Bergmann , Jann Horn , John Hubbard , Jesper Dangaard Brouer , Nicholas Piggin , =?iso-8859-1?B?Suly9G1l?= Glisse , Mathieu Desnoyers , kvm-ppc@vger.kernel.org, Thomas Gleixner , Reza Arbab , Allison Randal , Christian Brauner , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Logan Gunthorpe , Souptick Joarder , Andrew Morton , linuxppc-dev@lists.ozlabs.org, Roman Gushchin , "Kirill A. Shutemov" , Al Viro Message-ID: <20191004114236.oDY659LYc7ptMCatBurayTpAdX6WDzkIO9er6dGCed8@z> On Thu, Oct 03, 2019 at 05:36:31PM -0300, Leonardo Bras wrote: > > Also, I'm not sure I understand things properly. > >=20 > > So serialize_against_pte_lookup() wants to wait for all currently > > out-standing __find_linux_pte() instances (which are very similar to > > gup_fast). > >=20 > > It seems to want to do this before flushing the THP TLB for some reason; > > why? Should not THP observe the normal page table freeing rules which > > includes a RCU-like grace period like this already. > >=20 > > Why is THP special here? This doesn't seem adequately explained. >=20 > "It's necessary to monitor lockless pagetable walks, in order to avoid > doing THP splitting/collapsing during them." >=20 > If a there is a THP split/collapse during the lockless pagetable walk, > the returned ptep can be a pointing to an invalid pte.=20 So the whole premise of lockless page-table walks (gup_fast) is that it can work on in-flux page-tables. Specifically gup_fast() never returns PTEs, only struct page *, and only if it can increment the page refcount. In order to enable this, page-table pages are RCU(-like) freed, such that even if we access page-tables that have (concurrently) been unlinked, we'll not UaF (see asm-generic/tlb.h, the comment at HAVE_RCU_TABLE_FREE). IOW, the worst case if not getting a struct page *. I really don't see how THP splitting/collapsing is special here, either we see the PMD and find a struct page * or we see a PTE and find the same struct page * (compound page head). The only thing that needs to be guaranteed is that both PTEs and PMD page-tables are valid. Is this not so? > To avoid that, the pmd is updated, then serialize_against_pte_lookup is > ran. Serialize runs a do_nothing in all cpu in cpu_mask.=20 >=20 > So, after all cpus finish running do_nothing(), there is a guarantee > that if there is any 'lockless pagetable walk' it is running on top of > a updated version of this pmd, and so, collapsing/splitting THP is > safe. But why would it matter?! It would find the same struct page * through either version of the page-tables. *confused* > > Also, specifically to munmap(), this seems entirely superfluous, > > munmap() uses the normal page-table freeing code and should be entirely > > fine without additional waiting. >=20 > To be honest, I remember it being needed in munmap case, but I really > don't remember the details. I will take a deeper look and come back > with this answer.=20 munmap does normal mmu_gather page-table teardown, the THP PMD should be RCU-like freed just like any other PMD. Which should be perfectly safe vs lockless page-table walks. If you can find anything there that isn't right, please explain that in detail and we'll need to look hard at fixing _that_. > > Furthermore, Power never accurately tracks mm_cpumask(), so using that > > makes the whole thing more expensive than it needs to be. Also, I > > suppose that is buggered vs file backed THP. >=20 > That accuracy of mm_cpumask is above my knowledge right now. =3D) Basically PowerPC only ever sets bits in there, unlike x86 that also clears bits (at expense, but it's worth it for us).