| Message ID | 20221128180252.1684965-2-jannh@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/3] mm/khugepaged: Take the right locks for page table retraction | expand |
On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@google.com> 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? > > Cc: stable@kernel.org > Fixes: ba76149f47d8 ("thp: khugepaged") > Acked-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Jann Horn <jannh@google.com> > --- > v4: > - added ack from David Hildenbrand > - made commit message more verbose > > include/asm-generic/tlb.h | 4 ++++ > mm/khugepaged.c | 2 ++ > mm/mmu_gather.c | 4 +--- > 3 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h > index 492dce43236ea..cab7cfebf40bd 100644 > --- a/include/asm-generic/tlb.h > +++ b/include/asm-generic/tlb.h > @@ -222,12 +222,16 @@ extern void tlb_remove_table(struct mmu_gather *tlb, void *table); > #define tlb_needs_table_invalidate() (true) > #endif > > +void tlb_remove_table_sync_one(void); > + > #else > > #ifdef tlb_needs_table_invalidate > #error tlb_needs_table_invalidate() requires MMU_GATHER_RCU_TABLE_FREE > #endif > > +static inline void tlb_remove_table_sync_one(void) { } > + > #endif /* CONFIG_MMU_GATHER_RCU_TABLE_FREE */ > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 674b111a24fa7..c3d3ce596bff7 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -1057,6 +1057,7 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address, > _pmd = pmdp_collapse_flush(vma, address, pmd); > spin_unlock(pmd_ptl); > mmu_notifier_invalidate_range_end(&range); > + tlb_remove_table_sync_one(); > > spin_lock(pte_ptl); > result = __collapse_huge_page_isolate(vma, address, pte, cc, > @@ -1415,6 +1416,7 @@ static void collapse_and_free_pmd(struct mm_struct *mm, struct vm_area_struct *v > lockdep_assert_held_write(&vma->anon_vma->root->rwsem); > > pmd = pmdp_collapse_flush(vma, addr, pmdp); > + tlb_remove_table_sync_one(); > mm_dec_nr_ptes(mm); > page_table_check_pte_clear_range(mm, addr, pmd); > pte_free(mm, pmd_pgtable(pmd)); > diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c > index add4244e5790d..3a2c3f8cad2fe 100644 > --- a/mm/mmu_gather.c > +++ b/mm/mmu_gather.c > @@ -153,7 +153,7 @@ static void tlb_remove_table_smp_sync(void *arg) > /* Simply deliver the interrupt */ > } > > -static void tlb_remove_table_sync_one(void) > +void tlb_remove_table_sync_one(void) > { > /* > * This isn't an RCU grace period and hence the page-tables cannot be > @@ -177,8 +177,6 @@ static void tlb_remove_table_free(struct mmu_table_batch *batch) > > #else /* !CONFIG_MMU_GATHER_RCU_TABLE_FREE */ > > -static void tlb_remove_table_sync_one(void) { } > - > static void tlb_remove_table_free(struct mmu_table_batch *batch) > { > __tlb_remove_table_free(batch); > -- > 2.38.1.584.g0f3c55d4c2-goog >
On Mon, Nov 28, 2022 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@google.com> 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.
On Mon, Nov 28, 2022 at 11:57 AM Jann Horn <jannh@google.com> wrote: > > On Mon, Nov 28, 2022 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@google.com> 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. Reviewed-by: Yang Shi <shy828301@gmail.com>
On Mon, Nov 28, 2022 at 9:10 PM Yang Shi <shy828301@gmail.com> wrote: > On Mon, Nov 28, 2022 at 11:57 AM Jann Horn <jannh@google.com> wrote: > > > > On Mon, Nov 28, 2022 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@google.com> 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. > Reviewed-by: Yang Shi <shy828301@gmail.com> Thanks!
On Mon, Nov 28, 2022 at 08:56:54PM +0100, Jann Horn wrote: > On Mon, Nov 28, 2022 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@google.com> 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. True. I think I understand what Yang is confused about and I had the same question (asked in the old threads but didn't yet got a confirmation), since I think arm64 didn't use IPI for tlb is also true (according to the arm64 version of __flush_tlb_range), so PPC doesn't seem to be the only one. I mentioned PPC only because I saw the comment in mmu_gather.c: * Architectures that do not have this (PPC) need to delay the freeing by some * other means, this is that means. So I think it's obsolete. In short, IIUC there's just an implicit dependency that any !MMU_GATHER_RCU_TABLE_FREE arch must require IPI for tlb flush (not vice versa, hence arm64 can have RCU_TABLE_FREE), or something could be broken.
On Mon, Nov 28, 2022 at 12:12 PM Jann Horn <jannh@google.com> wrote: > > On Mon, Nov 28, 2022 at 9:10 PM Yang Shi <shy828301@gmail.com> wrote: > > On Mon, Nov 28, 2022 at 11:57 AM Jann Horn <jannh@google.com> wrote: > > > > > > On Mon, Nov 28, 2022 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > > > On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@google.com> 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@gmail.com> > > Thanks!
On Mon, Nov 28, 2022 at 11:10 PM Yang Shi <shy828301@gmail.com> wrote: > > On Mon, Nov 28, 2022 at 12:12 PM Jann Horn <jannh@google.com> wrote: > > > > On Mon, Nov 28, 2022 at 9:10 PM Yang Shi <shy828301@gmail.com> wrote: > > > On Mon, Nov 28, 2022 at 11:57 AM Jann Horn <jannh@google.com> wrote: > > > > > > > > On Mon, Nov 28, 2022 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > > > > > On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@google.com> 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. OK, I will rewrite the commit message to describe the overall problem, including the part addressed by your patch.
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h index 492dce43236ea..cab7cfebf40bd 100644 --- a/include/asm-generic/tlb.h +++ b/include/asm-generic/tlb.h @@ -222,12 +222,16 @@ extern void tlb_remove_table(struct mmu_gather *tlb, void *table); #define tlb_needs_table_invalidate() (true) #endif +void tlb_remove_table_sync_one(void); + #else #ifdef tlb_needs_table_invalidate #error tlb_needs_table_invalidate() requires MMU_GATHER_RCU_TABLE_FREE #endif +static inline void tlb_remove_table_sync_one(void) { } + #endif /* CONFIG_MMU_GATHER_RCU_TABLE_FREE */ diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 674b111a24fa7..c3d3ce596bff7 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -1057,6 +1057,7 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address, _pmd = pmdp_collapse_flush(vma, address, pmd); spin_unlock(pmd_ptl); mmu_notifier_invalidate_range_end(&range); + tlb_remove_table_sync_one(); spin_lock(pte_ptl); result = __collapse_huge_page_isolate(vma, address, pte, cc, @@ -1415,6 +1416,7 @@ static void collapse_and_free_pmd(struct mm_struct *mm, struct vm_area_struct *v lockdep_assert_held_write(&vma->anon_vma->root->rwsem); pmd = pmdp_collapse_flush(vma, addr, pmdp); + tlb_remove_table_sync_one(); mm_dec_nr_ptes(mm); page_table_check_pte_clear_range(mm, addr, pmd); pte_free(mm, pmd_pgtable(pmd)); diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c index add4244e5790d..3a2c3f8cad2fe 100644 --- a/mm/mmu_gather.c +++ b/mm/mmu_gather.c @@ -153,7 +153,7 @@ static void tlb_remove_table_smp_sync(void *arg) /* Simply deliver the interrupt */ } -static void tlb_remove_table_sync_one(void) +void tlb_remove_table_sync_one(void) { /* * This isn't an RCU grace period and hence the page-tables cannot be @@ -177,8 +177,6 @@ static void tlb_remove_table_free(struct mmu_table_batch *batch) #else /* !CONFIG_MMU_GATHER_RCU_TABLE_FREE */ -static void tlb_remove_table_sync_one(void) { } - static void tlb_remove_table_free(struct mmu_table_batch *batch) { __tlb_remove_table_free(batch);
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. Cc: stable@kernel.org Fixes: ba76149f47d8 ("thp: khugepaged") Acked-by: David Hildenbrand <david@redhat.com> Signed-off-by: Jann Horn <jannh@google.com> --- v4: - added ack from David Hildenbrand - made commit message more verbose include/asm-generic/tlb.h | 4 ++++ mm/khugepaged.c | 2 ++ mm/mmu_gather.c | 4 +--- 3 files changed, 7 insertions(+), 3 deletions(-)