Message-ID: <20191004114236.GD19463@hirez.programming.kicks-ass.net>
Date:   Fri, 4 Oct 2019 13:42:36 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Leonardo Bras <leonardo@...ux.ibm.com>
Cc:     Song Liu <songliubraving@...com>, Michal Hocko <mhocko@...e.com>,
        "Dmitry V. Levin" <ldv@...linux.org>,
        Keith Busch <keith.busch@...el.com>, linux-mm@...ck.org,
        Paul Mackerras <paulus@...ba.org>,
        Christoph Lameter <cl@...ux.com>,
        Ira Weiny <ira.weiny@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Elena Reshetova <elena.reshetova@...el.com>,
        linux-arch@...r.kernel.org, Santosh Sivaraj <santosh@...six.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Vlastimil Babka <vbabka@...e.cz>,
        Mahesh Salgaonkar <mahesh@...ux.vnet.ibm.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Ingo Molnar <mingo@...nel.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Ralph Campbell <rcampbell@...dia.com>,
        Arnd Bergmann <arnd@...db.de>, Jann Horn <jannh@...gle.com>,
        John Hubbard <jhubbard@...dia.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Nicholas Piggin <npiggin@...il.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        kvm-ppc@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Reza Arbab <arbab@...ux.ibm.com>,
        Allison Randal <allison@...utok.net>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org,
        Logan Gunthorpe <logang@...tatee.com>,
        Souptick Joarder <jrdr.linux@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linuxppc-dev@...ts.ozlabs.org, Roman Gushchin <guro@...com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v5 00/11] Introduces new count-based method for tracking
 lockless pagetable walks

On Thu, Oct 03, 2019 at 05:36:31PM -0300, Leonardo Bras wrote:

> > Also, I'm not sure I understand things properly.
> > 
> > So serialize_against_pte_lookup() wants to wait for all currently
> > out-standing __find_linux_pte() instances (which are very similar to
> > gup_fast).
> > 
> > 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.
> > 
> > Why is THP special here? This doesn't seem adequately explained.
> 
> "It's necessary to monitor lockless pagetable walks, in order to avoid
> doing THP splitting/collapsing during them."
> 
> If a there is a THP split/collapse during the lockless pagetable walk,
> the returned ptep can be a pointing to an invalid pte. 

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. 
> 
> 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.
> 
> 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. 

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.
> 
> That accuracy of mm_cpumask is above my knowledge right now. =)

Basically PowerPC only ever sets bits in there, unlike x86 that also
clears bits (at expense, but it's worth it for us).

Powered by blists - more mailing lists