[PATCH 5.10 0/2] mm/truncate: fix issue in ext4_set_page_dirty()

[PATCH 5.10 0/2] mm/truncate: fix issue in ext4_set_page_dirty()

[Roman Smirnov]

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(-)

-- 
2.34.1

[PATCH 5.10 1/2] mm/truncate: Inline invalidate_complete_page() into its one caller

[Roman Smirnov]

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
-- 
2.34.1

[PATCH 5.10 2/2] mm/truncate: Replace page_mapped() call in invalidate_inode_page()

[Roman Smirnov]

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;
-- 
2.34.1

Re: [PATCH 5.10 0/2] mm/truncate: fix issue in ext4_set_page_dirty()

[Matthew Wilcox]

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
> 

Re: [PATCH 5.10 0/2] mm/truncate: fix issue in ext4_set_page_dirty()

[Roman Smirnov]

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.