Nicholas Piggin <npig...@gmail.com> writes: > Commit 1b2443a547f9 ("powerpc/book3s64: Avoid multiple endian conversion > in pte helpers") changed the actual bitwise tests in pte_access_permitted > by using pte_write() and pte_present() helpers rather than raw bitwise > testing _PAGE_WRITE and _PAGE_PRESENT bits. > > The pte_present change now returns true for ptes which are !_PAGE_PRESENT > and _PAGE_INVALID, which is the combination used by pmdp_invalidate to > synchronize access from lock-free lookups. pte_access_permitted is used by > pmd_access_permitted, so allowing GUP lock free access to proceed with > such PTEs breaks this synchronisation. > > This bug has been observed on HPT host, with random crashes and corruption > in guests, usually together with bad PMD messages in the host. > > Fix this by adding an explicit check in pmd_access_permitted, and > documenting the condition explicitly. > > The pte_write() change should be okay, and would prevent GUP from falling > back to the slow path when encountering savedwrite ptes, which matches > what x86 (that does not implement savedwrite) does. >
I guess we are doing the find_linux_pte change in another patch. Reviewed-by: Aneesh Kumar K.V <aneesh.ku...@linux.ibm.com> > Fixes: 1b2443a547f9 ("powerpc/book3s64: Avoid multiple endian conversion in > pte helpers") > Cc: Aneesh Kumar K.V <aneesh.ku...@linux.ibm.com> > Cc: Christophe Leroy <christophe.le...@c-s.fr> > Signed-off-by: Nicholas Piggin <npig...@gmail.com> > --- > arch/powerpc/include/asm/book3s/64/pgtable.h | 19 ++++++++++++++++++- > arch/powerpc/mm/book3s64/pgtable.c | 3 +++ > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h > b/arch/powerpc/include/asm/book3s/64/pgtable.h > index 7dede2e34b70..aaa72aa1b765 100644 > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h > @@ -1092,7 +1092,24 @@ static inline int pmd_protnone(pmd_t pmd) > #define pmd_access_permitted pmd_access_permitted > static inline bool pmd_access_permitted(pmd_t pmd, bool write) > { > - return pte_access_permitted(pmd_pte(pmd), write); > + pte_t pte = pmd_pte(pmd); > + unsigned long pteval = pte_val(pte); > + > + /* > + * pmdp_invalidate sets this combination (that is not caught by > + * !pte_present() check in pte_access_permitted), to prevent > + * lock-free lookups, as part of the serialize_against_pte_lookup() > + * synchronisation. > + * > + * This check inadvertently catches the case where the PTE's hardware > + * PRESENT bit is cleared while TLB is flushed, to work around > + * hardware TLB issues. This is suboptimal, but should not be hit > + * frequently and should be harmless. > + */ > + if ((pteval & _PAGE_INVALID) && !(pteval & _PAGE_PRESENT)) > + return false; > + > + return pte_access_permitted(pte, write); > } > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > diff --git a/arch/powerpc/mm/book3s64/pgtable.c > b/arch/powerpc/mm/book3s64/pgtable.c > index 16bda049187a..ff98b663c83e 100644 > --- a/arch/powerpc/mm/book3s64/pgtable.c > +++ b/arch/powerpc/mm/book3s64/pgtable.c > @@ -116,6 +116,9 @@ pmd_t pmdp_invalidate(struct vm_area_struct *vma, > unsigned long address, > /* > * This ensures that generic code that rely on IRQ disabling > * to prevent a parallel THP split work as expected. > + * > + * Marking the entry with _PAGE_INVALID && ~_PAGE_PRESENT requires > + * a special case check in pmd_access_permitted. > */ > serialize_against_pte_lookup(vma->vm_mm); > return __pmd(old_pmd); > -- > 2.20.1