Syzkaller reports warning in ext4_set_page_dirty() in 5.10 stable releases. The problem can be fixed by the following patches which can be cleanly applied to the 5.10 branch.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Link: https://syzkaller.appspot.com/bug?extid=02f21431b65c214aa1d6
Matthew Wilcox (Oracle) (2): mm/truncate: Inline invalidate_complete_page() into its one caller mm/truncate: Replace page_mapped() call in invalidate_inode_page()
kernel/futex/core.c | 2 +- mm/truncate.c | 34 +++++++--------------------------- 2 files changed, 8 insertions(+), 28 deletions(-)
From: "Matthew Wilcox (Oracle)" willy@infradead.org
Commit 1b8ddbeeb9b819e62b7190115023ce858a159f5c upstream.
invalidate_inode_page() is the only caller of invalidate_complete_page() and inlining it reveals that the first check is unnecessary (because we hold the page locked, and we just retrieved the mapping from the page). Actually, it does make a difference, in that tail pages no longer fail at this check, so it's now possible to remove a tail page from a mapping.
Signed-off-by: Matthew Wilcox (Oracle) willy@infradead.org Reviewed-by: John Hubbard jhubbard@nvidia.com Reviewed-by: Christoph Hellwig hch@lst.de Signed-off-by: Roman Smirnov r.smirnov@omp.ru --- kernel/futex/core.c | 2 +- mm/truncate.c | 31 +++++-------------------------- 2 files changed, 6 insertions(+), 27 deletions(-)
diff --git a/kernel/futex/core.c b/kernel/futex/core.c index cde0ca876b93..cbbebc3de1d3 100644 --- a/kernel/futex/core.c +++ b/kernel/futex/core.c @@ -578,7 +578,7 @@ static int get_futex_key(u32 __user *uaddr, bool fshared, union futex_key *key, * found it, but truncated or holepunched or subjected to * invalidate_complete_page2 before we got the page lock (also * cases which we are happy to fail). And we hold a reference, - * so refcount care in invalidate_complete_page's remove_mapping + * so refcount care in invalidate_inode_page's remove_mapping * prevents drop_caches from setting mapping to NULL beneath us. * * The case we do have to guard against is when memory pressure made diff --git a/mm/truncate.c b/mm/truncate.c index 8914ca4ce4b1..03998fd86e4a 100644 --- a/mm/truncate.c +++ b/mm/truncate.c @@ -190,30 +190,6 @@ static void truncate_cleanup_page(struct page *page) ClearPageMappedToDisk(page); }
-/* - * This is for invalidate_mapping_pages(). That function can be called at - * any time, and is not supposed to throw away dirty pages. But pages can - * be marked dirty at any time too, so use remove_mapping which safely - * discards clean, unused pages. - * - * Returns non-zero if the page was successfully invalidated. - */ -static int -invalidate_complete_page(struct address_space *mapping, struct page *page) -{ - int ret; - - if (page->mapping != mapping) - return 0; - - if (page_has_private(page) && !try_to_release_page(page, 0)) - return 0; - - ret = remove_mapping(mapping, page); - - return ret; -} - int truncate_inode_page(struct address_space *mapping, struct page *page) { VM_BUG_ON_PAGE(PageTail(page), page); @@ -258,7 +234,10 @@ int invalidate_inode_page(struct page *page) return 0; if (page_mapped(page)) return 0; - return invalidate_complete_page(mapping, page); + if (page_has_private(page) && !try_to_release_page(page, 0)) + return 0; + + return remove_mapping(mapping, page); }
/** @@ -645,7 +624,7 @@ void invalidate_mapping_pagevec(struct address_space *mapping, }
/* - * This is like invalidate_complete_page(), except it ignores the page's + * This is like invalidate_inode_page(), except it ignores the page's * refcount. We do this because invalidate_inode_pages2() needs stronger * invalidation guarantees, and cannot afford to leave pages behind because * shrink_page_list() has a temp ref on them, or because they're transiently
From: "Matthew Wilcox (Oracle)" willy@infradead.org
Commit e41c81d0d30e1a6ebf408feaf561f80cac4457dc upstream.
folio_mapped() is expensive because it has to check each page's mapcount field. A cheaper check is whether there are any extra references to the page, other than the one we own, one from the page private data and the ones held by the page cache.
The call to remove_mapping() will fail in any case if it cannot freeze the refcount, but failing here avoids cycling the i_pages spinlock.
[Roman: replaced folio_ref_count() call with page_ref_count(), folio_nr_pages() call with compound_nr() and folio_has_private() call with page_has_private()]
Signed-off-by: Matthew Wilcox (Oracle) willy@infradead.org Reviewed-by: Miaohe Lin linmiaohe@huawei.com Signed-off-by: Roman Smirnov r.smirnov@omp.ru --- mm/truncate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/truncate.c b/mm/truncate.c index 03998fd86e4a..72d6c62756fd 100644 --- a/mm/truncate.c +++ b/mm/truncate.c @@ -232,7 +232,8 @@ int invalidate_inode_page(struct page *page) return 0; if (PageDirty(page) || PageWriteback(page)) return 0; - if (page_mapped(page)) + if (page_ref_count(page) > + compound_nr(page) + page_has_private(page) + 1) return 0; if (page_has_private(page) && !try_to_release_page(page, 0)) return 0;
On Thu, Jan 11, 2024 at 02:37:45PM +0000, Roman Smirnov wrote:
Syzkaller reports warning in ext4_set_page_dirty() in 5.10 stable releases. The problem can be fixed by the following patches which can be cleanly applied to the 5.10 branch.
I do not understand the crash, and I do not understand why this patch would fix it. Can you explain either?
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Link: https://syzkaller.appspot.com/bug?extid=02f21431b65c214aa1d6
Matthew Wilcox (Oracle) (2): mm/truncate: Inline invalidate_complete_page() into its one caller mm/truncate: Replace page_mapped() call in invalidate_inode_page()
kernel/futex/core.c | 2 +- mm/truncate.c | 34 +++++++--------------------------- 2 files changed, 8 insertions(+), 28 deletions(-)
-- 2.34.1
On Thu, 11 Jan 2024 15:31:12 +0000, Matthew Wilcox wrote:
I do not understand the crash, and I do not understand why this patch would fix it. Can you explain either?
The WARNING appears in the following location: https://elixir.bootlin.com/linux/v5.10.205/source/fs/ext4/inode.c#L3693
Reverse bisection pointed at the 2nd patch as a fix, but after backporting this patch to 5.10 branch I still hit the WARNING. I noticed that there was some missing code compared to the original patch:
if (folio_has_private(folio) && !filemap_release_folio(folio, 0)) return 0;
Then I found a patch with this code before using folio, applied it, and tests showed the WARNING disappeared. I also used the linux test project to make sure nothing was broken. I'll try to dig a little deeper and explain the crash.
Thanks for the reply.
linux-stable-mirror@lists.linaro.org
-
Matthew Wilcox -
Roman Smirnov