Re: [PATCH v4 2/3] mm/khugepaged: Fix GUP-fast interaction by sending IPI

[Yang Shi]

On Mon, Nov 28, 2022 at 12:12 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
>
> On Mon, Nov 28, 2022 at 9:10 PM Yang Shi <shy828301@xxxxxxxxx> wrote:
> > On Mon, Nov 28, 2022 at 11:57 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Nov 28, 2022 at 8:54 PM Yang Shi <shy828301@xxxxxxxxx> wrote:
> > > >
> > > > On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
> > > > >
> > > > > Since commit 70cbc3cc78a99 ("mm: gup: fix the fast GUP race against THP
> > > > > collapse"), the lockless_pages_from_mm() fastpath rechecks the pmd_t to
> > > > > ensure that the page table was not removed by khugepaged in between.
> > > > >
> > > > > However, lockless_pages_from_mm() still requires that the page table is not
> > > > > concurrently freed or reused to store non-PTE data. Otherwise, problems
> > > > > can occur because:
> > > > >
> > > > >  - deposited page tables can be freed when a THP page somewhere in the
> > > > >    mm is removed
> > > > >  - some architectures store non-PTE information inside deposited page
> > > > >    tables (see radix__pgtable_trans_huge_deposit())
> > > > >
> > > > > Additionally, lockless_pages_from_mm() is also somewhat brittle with
> > > > > regards to page tables being repeatedly moved back and forth, but
> > > > > that shouldn't be an issue in practice.
> > > > >
> > > > > Fix it by sending IPIs (if the architecture uses
> > > > > semi-RCU-style page table freeing) before freeing/reusing page tables.
> > > > >
> > > > > As noted in mm/gup.c, on configs that define CONFIG_HAVE_FAST_GUP,
> > > > > there are two possible cases:
> > > > >
> > > > >  1. CONFIG_MMU_GATHER_RCU_TABLE_FREE is set, causing
> > > > >     tlb_remove_table_sync_one() to send an IPI to synchronize with
> > > > >     lockless_pages_from_mm().
> > > > >  2. CONFIG_MMU_GATHER_RCU_TABLE_FREE is unset, indicating that all
> > > > >     TLB flushes are already guaranteed to send IPIs.
> > > > >     tlb_remove_table_sync_one() will do nothing, but we've already
> > > > >     run pmdp_collapse_flush(), which did a TLB flush, which must have
> > > > >     involved IPIs.
> > > >
> > > > I'm trying to catch up with the discussion after the holiday break. I
> > > > understand you switched from always allocating a new page table page
> > > > (we decided before) to sending IPIs to serialize against fast-GUP,
> > > > this is fine to me.
> > > >
> > > > So the code now looks like:
> > > >     pmdp_collapse_flush()
> > > >     sending IPI
> > > >
> > > > But the missing part is how we reached "TLB flushes are already
> > > > guaranteed to send IPIs" when CONFIG_MMU_GATHER_RCU_TABLE_FREE is
> > > > unset? ARM64 doesn't do it IIRC. Or did I miss something?
> > >
> > > From arch/arm64/Kconfig:
> > >
> > > select MMU_GATHER_RCU_TABLE_FREE
> > >
> > > CONFIG_MMU_GATHER_RCU_TABLE_FREE is not a config option that the user
> > > can freely toggle; it is an option selected by the architecture.
> >
> > Aha, I see :-) BTW, shall we revert "mm: gup: fix the fast GUP race
> > against THP collapse"? It seems not necessary anymore if this approach
> > is used IIUC.
>
> Yeah, I agree.

Since this patch could solve two problems: the use-after-free of the
data page (pinned by fast-GUP) and the page table page and my patch
will be reverted, so could you please catch both issues in this
patch's commit log? I'd like to preserve the description of the issue
fixed by my patch. I think that it is helpful to see the information
about all the fixed problems in one commit instead of digging into
another reverted commit.

>
> > Reviewed-by: Yang Shi <shy828301@xxxxxxxxx>
>
> Thanks!