| Message ID | 20211022074619.57355-1-andrea.righi@canonical.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: fix sleeping copy_huge_page called from atomic context | expand |
On Fri, Oct 22, 2021 at 09:46:19AM +0200, Andrea Righi wrote: > copy_huge_page() can be called with mapping->private_lock held from > __buffer_migrate_page() -> migrate_page_copy(), so it is not safe to > do a cond_resched() in this context. > > Introduce migrate_page_copy_nowait() and copy_huge_page_nowait() > variants that can be used from an atomic context. I think this is a consequence of THPs being created when they should not be. This is the wrong way to fix this problem; and I suspect it may already be fixed at least in -mm. We should have taken this path: if (!page_has_buffers(page)) return migrate_page(mapping, newpage, page, mode); but since we didn't, we can infer that there's a THP which has buffers (this should never occur). It's the same root cause as the invalidatepage problem, just with a very different signature.
On Fri, Oct 22, 2021 at 4:16 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Oct 22, 2021 at 09:46:19AM +0200, Andrea Righi wrote: > > copy_huge_page() can be called with mapping->private_lock held from > > __buffer_migrate_page() -> migrate_page_copy(), so it is not safe to > > do a cond_resched() in this context. > > > > Introduce migrate_page_copy_nowait() and copy_huge_page_nowait() > > variants that can be used from an atomic context. > > I think this is a consequence of THPs being created when they should not > be. This is the wrong way to fix this problem; and I suspect it may > already be fixed at least in -mm. We should have taken this path: > > if (!page_has_buffers(page)) > return migrate_page(mapping, newpage, page, mode); > > but since we didn't, we can infer that there's a THP which has buffers > (this should never occur). It's the same root cause as the invalidatepage > problem, just with a very different signature. Yeah, exactly. And I replied to that syzbot report a few days ago (https://lore.kernel.org/linux-mm/CAHbLzkoFaowaG8AU6tg_WMPdjcAdyE+Wafs7TJz1Z23TRg_d8A@mail.gmail.com/) with the same conclusion. I'm not sure why Hugh didn't submit his patch, maybe he was waiting for the test result from the bug reporter of that invalidatepage issue? It should be fine, the fix is quite straightforward IMHO. >
On Fri, Oct 22, 2021 at 10:38 AM Yang Shi <shy828301@gmail.com> wrote: > > On Fri, Oct 22, 2021 at 4:16 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Fri, Oct 22, 2021 at 09:46:19AM +0200, Andrea Righi wrote: > > > copy_huge_page() can be called with mapping->private_lock held from > > > __buffer_migrate_page() -> migrate_page_copy(), so it is not safe to > > > do a cond_resched() in this context. > > > > > > Introduce migrate_page_copy_nowait() and copy_huge_page_nowait() > > > variants that can be used from an atomic context. > > > > I think this is a consequence of THPs being created when they should not > > be. This is the wrong way to fix this problem; and I suspect it may > > already be fixed at least in -mm. We should have taken this path: > > > > if (!page_has_buffers(page)) > > return migrate_page(mapping, newpage, page, mode); > > > > but since we didn't, we can infer that there's a THP which has buffers > > (this should never occur). It's the same root cause as the invalidatepage > > problem, just with a very different signature. > > Yeah, exactly. And I replied to that syzbot report a few days ago > (https://lore.kernel.org/linux-mm/CAHbLzkoFaowaG8AU6tg_WMPdjcAdyE+Wafs7TJz1Z23TRg_d8A@mail.gmail.com/) > with the same conclusion. > > I'm not sure why Hugh didn't submit his patch, maybe he was waiting > for the test result from the bug reporter of that invalidatepage > issue? It should be fine, the fix is quite straightforward IMHO. Anyway if Hugh doesn't have time to do it, I could prepare the patch for formal review. > > >
diff --git a/include/linux/migrate.h b/include/linux/migrate.h index c8077e936691..3dc6dab9a3f7 100644 --- a/include/linux/migrate.h +++ b/include/linux/migrate.h @@ -52,7 +52,15 @@ extern struct page *alloc_migration_target(struct page *page, unsigned long priv extern int isolate_movable_page(struct page *page, isolate_mode_t mode); extern void migrate_page_states(struct page *newpage, struct page *page); -extern void migrate_page_copy(struct page *newpage, struct page *page); +extern void __migrate_page_copy(struct page *newpage, struct page *page, bool atomic); +static inline void migrate_page_copy(struct page *newpage, struct page *page) +{ + return __migrate_page_copy(newpage, page, false); +} +static inline void migrate_page_copy_nowait(struct page *newpage, struct page *page) +{ + return __migrate_page_copy(newpage, page, true); +} extern int migrate_huge_page_move_mapping(struct address_space *mapping, struct page *newpage, struct page *page); extern int migrate_page_move_mapping(struct address_space *mapping, diff --git a/include/linux/mm.h b/include/linux/mm.h index 73a52aba448f..1c96bb084366 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -907,7 +907,15 @@ void __put_page(struct page *page); void put_pages_list(struct list_head *pages); void split_page(struct page *page, unsigned int order); -void copy_huge_page(struct page *dst, struct page *src); +void __copy_huge_page(struct page *dst, struct page *src, bool atomic); +static inline void copy_huge_page(struct page *dst, struct page *src) +{ + __copy_huge_page(dst, src, false); +} +static inline void copy_huge_page_nowait(struct page *dst, struct page *src) +{ + __copy_huge_page(dst, src, true); +} /* * Compound pages have a destructor function. Provide a diff --git a/mm/migrate.c b/mm/migrate.c index 1852d787e6ab..d8bc0586d157 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -613,16 +613,16 @@ void migrate_page_states(struct page *newpage, struct page *page) } EXPORT_SYMBOL(migrate_page_states); -void migrate_page_copy(struct page *newpage, struct page *page) +void __migrate_page_copy(struct page *newpage, struct page *page, bool atomic) { if (PageHuge(page) || PageTransHuge(page)) - copy_huge_page(newpage, page); + __copy_huge_page(newpage, page, atomic); else copy_highpage(newpage, page); migrate_page_states(newpage, page); } -EXPORT_SYMBOL(migrate_page_copy); +EXPORT_SYMBOL(__migrate_page_copy); /************************************************************ * Migration functions @@ -755,7 +755,7 @@ static int __buffer_migrate_page(struct address_space *mapping, } while (bh != head); if (mode != MIGRATE_SYNC_NO_COPY) - migrate_page_copy(newpage, page); + migrate_page_copy_nowait(newpage, page); else migrate_page_states(newpage, page); diff --git a/mm/util.c b/mm/util.c index bacabe446906..f84e65643d1d 100644 --- a/mm/util.c +++ b/mm/util.c @@ -750,12 +750,13 @@ int __page_mapcount(struct page *page) } EXPORT_SYMBOL_GPL(__page_mapcount); -void copy_huge_page(struct page *dst, struct page *src) +void __copy_huge_page(struct page *dst, struct page *src, bool atomic) { unsigned i, nr = compound_nr(src); for (i = 0; i < nr; i++) { - cond_resched(); + if (!atomic) + cond_resched(); copy_highpage(nth_page(dst, i), nth_page(src, i)); } }
copy_huge_page() can be called with mapping->private_lock held from __buffer_migrate_page() -> migrate_page_copy(), so it is not safe to do a cond_resched() in this context. Introduce migrate_page_copy_nowait() and copy_huge_page_nowait() variants that can be used from an atomic context. The downside of this change is that we may experience temporary soft lockups when copying large huge pages in very slow systems, but this allows to prevent potential deadlocks. Link: https://syzkaller.appspot.com/bug?id=683b472eb7539d56da69de85f4bfb4b9af67f7ec Fixes: 79789db03fdd ("mm: Make copy_huge_page() always available") Signed-off-by: Andrea Righi <andrea.righi@canonical.com> --- include/linux/migrate.h | 10 +++++++++- include/linux/mm.h | 10 +++++++++- mm/migrate.c | 8 ++++---- mm/util.c | 5 +++-- 4 files changed, 25 insertions(+), 8 deletions(-)