Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse

[Yang Shi <shy828301@gmail.com>]

On Mon, Sep 5, 2022 at 1:56 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Yang Shi <shy828301@gmail.com> writes:
>
> >
> > On Fri, Sep 2, 2022 at 9:00 AM Peter Xu <peterx@redhat.com> wrote:
> >>
> >> On Thu, Sep 01, 2022 at 04:50:45PM -0700, Yang Shi wrote:
> >> > On Thu, Sep 1, 2022 at 4:26 PM Peter Xu <peterx@redhat.com> wrote:
> >> > >
> >> > > Hi, Yang,
> >> > >
> >> > > On Thu, Sep 01, 2022 at 03:27:07PM -0700, Yang Shi wrote:
> >> > > > Since general RCU GUP fast was introduced in commit 2667f50e8b81 ("mm:
> >> > > > introduce a general RCU get_user_pages_fast()"), a TLB flush is no longer
> >> > > > sufficient to handle concurrent GUP-fast in all cases, it only handles
> >> > > > traditional IPI-based GUP-fast correctly.
> >> > >
> >> > > If TLB flush (or, IPI broadcasts) used to work to protect against gup-fast,
> >> > > I'm kind of confused why it's not sufficient even if with RCU gup?  Isn't
> >> > > that'll keep working as long as interrupt disabled (which current fast-gup
> >> > > will still do)?
> >> >
> >> > Actually the wording was copied from David's commit log for his
> >> > PageAnonExclusive fix. My understanding is the IPI broadcast still
> >> > works, but it may not be supported by all architectures and not
> >> > preferred anymore. So we should avoid depending on IPI broadcast IIUC.
> >> >
> >> > >
> >> > > IIUC the issue is you suspect not all archs correctly implemented
> >> > > pmdp_collapse_flush(), or am I wrong?
> >> >
> >> > This is a possible fix, please see below for details.
> >> >
> >> > >
> >> > > > On architectures that send
> >> > > > an IPI broadcast on TLB flush, it works as expected.  But on the
> >> > > > architectures that do not use IPI to broadcast TLB flush, it may have
> >> > > > the below race:
> >> > > >
> >> > > >    CPU A                                          CPU B
> >> > > > THP collapse                                     fast GUP
> >> > > >                                               gup_pmd_range() <-- see valid pmd
> >> > > >                                                   gup_pte_range() <-- work on pte
> >> > > > pmdp_collapse_flush() <-- clear pmd and flush
> >> > > > __collapse_huge_page_isolate()
> >> > > >     check page pinned <-- before GUP bump refcount
> >> > > >                                                       pin the page
> >> > > >                                                       check PTE <-- no change
> >> > > > __collapse_huge_page_copy()
> >> > > >     copy data to huge page
> >> > > >     ptep_clear()
> >> > > > install huge pmd for the huge page
> >> > > >                                                       return the stale page
> >> > > > discard the stale page
> >> > > >
> >> > > > The race could be fixed by checking whether PMD is changed or not after
> >> > > > taking the page pin in fast GUP, just like what it does for PTE.  If the
> >> > > > PMD is changed it means there may be parallel THP collapse, so GUP
> >> > > > should back off.
> >> > >
> >> > > Could the race also be fixed by impl pmdp_collapse_flush() correctly for
> >> > > the archs that are missing? Do you know which arch(s) is broken with it?
> >> >
> >> > Yes, and this was suggested by me in the first place, but per the
> >> > suggestion from John and David, this is not the preferred way. I think
> >> > it is because:
> >> >
> >> > Firstly, using IPI to serialize against fast GUP is not recommended
> >> > anymore since fast GUP does check PTE then back off so we should avoid
> >> > it.
> >> > Secondly, if checking PMD then backing off could solve the problem,
> >> > why do we still need broadcast IPI? It doesn't sound performant.
> >> >
> >> > >
> >> > > It's just not clear to me whether this patch is an optimization or a fix,
> >> > > if it's a fix whether the IPI broadcast in ppc pmdp_collapse_flush() would
> >> > > still be needed.
> >> >
> >> > It is a fix and the fix will make IPI broadcast not useful anymore.
> >>
> >> How about another patch to remove the ppc impl too?  Then it can be a two
> >> patches series.
> >
> > BTW, I don't think we could remove the ppc implementation since it is
> > different from the generic pmdp_collapse_flush(), particularly for the
> > hash part IIUC.
> >
> > The generic version calls flush_tlb_range() -> hash__flush_tlb_range()
> > for hash, but the hash call is actually no-op. The ppc version calls
> > hash__pmdp_collapse_flush() -> flush_tlb_pmd_range(), which does
> > something useful.
> >
>
> We should actually rename flush_tlb_pmd_range(). It actually flush the
> hash page table entries.
>
> I will do the below patch for ppc64 to clarify this better

Thanks, Aneesh. It looks more readable. A follow-up question, I think
we could remove serialize_against_pte_lookup(), which just issues IPI
broadcast to run a dummy function. This IPI should not be needed
anymore with my patch. Of course, we need to keep the memory barrier.

>
> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> index 8b762f282190..fd30fa20c392 100644
> --- a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> @@ -112,13 +112,12 @@ static inline void hash__flush_tlb_kernel_range(unsigned long start,
>
>  struct mmu_gather;
>  extern void hash__tlb_flush(struct mmu_gather *tlb);
> -void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr);
>
>  #ifdef CONFIG_PPC_64S_HASH_MMU
>  /* Private function for use by PCI IO mapping code */
>  extern void __flush_hash_table_range(unsigned long start, unsigned long end);
> -extern void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd,
> -                               unsigned long addr);
> +extern void flush_hash_table_pmd_range(struct mm_struct *mm, pmd_t *pmd,
> +                                      unsigned long addr);
>  #else
>  static inline void __flush_hash_table_range(unsigned long start, unsigned long end) { }
>  #endif
> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c
> index ae008b9df0e6..f30131933a01 100644
> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c
> @@ -256,7 +256,7 @@ pmd_t hash__pmdp_collapse_flush(struct vm_area_struct *vma, unsigned long addres
>          * the __collapse_huge_page_copy can result in copying
>          * the old content.
>          */
> -       flush_tlb_pmd_range(vma->vm_mm, &pmd, address);
> +       flush_hash_table_pmd_range(vma->vm_mm, &pmd, address);
>         return pmd;
>  }
>
> diff --git a/arch/powerpc/mm/book3s64/hash_tlb.c b/arch/powerpc/mm/book3s64/hash_tlb.c
> index eb0bccaf221e..a64ea0a7ef96 100644
> --- a/arch/powerpc/mm/book3s64/hash_tlb.c
> +++ b/arch/powerpc/mm/book3s64/hash_tlb.c
> @@ -221,7 +221,7 @@ void __flush_hash_table_range(unsigned long start, unsigned long end)
>         local_irq_restore(flags);
>  }
>
> -void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr)
> +void flush_hash_table_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr)
>  {
>         pte_t *pte;
>         pte_t *start_pte;
>