[PATCH] fs/ntfs3: fix ABBA page-lock deadlock in ni_read_folio_cmpr()

[PATCH] fs/ntfs3: fix ABBA page-lock deadlock in ni_read_folio_cmpr()

[Eugene Susla]

ni_read_folio_cmpr() is called with the folio being read locked, then
locks every other page of the compression frame in ascending order via
ntfs_lock_new_page(), sleeping on each page lock. Two tasks reading
different pages of the same frame (e.g. two page faults on an mmap of a
compressed file) can each hold the page the other one needs next:

  task A: faults page 15, holds it, waits for page 0
  task B: faults page 0, holds it, waits for page 15

Both tasks sleep forever in D state:

  __filemap_get_folio_mpol+0x344/0x4e0
  ni_read_folio_cmpr+0x18b/0x490
  ntfs_read_folio+0x8c/0x110
  filemap_read_folio+0x1f/0xa0
  filemap_fault+0x7c0/0x1400

They cannot be killed, and suspend fails because they cannot be frozen.

The same lock order inversion exists between a reader and
ntfs_compress_write(), which locks the frame pages in ascending order
via ntfs_get_frame_pages().

Never sleep on the other page locks from the read path: pass FGP_NOWAIT
and, if a page is locked by someone else, decompress into a temporary
page instead, as is already done for pages that are uptodate. The data
in that page is discarded; the page is left to its lock holder. squashfs
does the same for the other pages of a compressed block.

ni_decompress_file() keeps blocking: it holds no page lock when it starts
locking a frame, so it cannot be part of such a cycle.

Reproduced under virtme-ng on v7.3-rc6 with a 64 MiB LZNT-compressed
file on a loop-mounted ntfs3 image: two processes faulting page 0 and
page 15 of every 64 KiB frame after a cache drop hang in the first pass
(5 of 5 runs). With this patch, 5 runs of 50 passes and one 16-process
run of 20 passes complete, the temporary-page fallback is taken about
46000 times in a 50-pass run, and the pages read and the file's cached
contents match the original data.

Link: https://syzkaller.appspot.com/bug?id=91960c6feb9ae216c9004c0f864f74aab1aebd5a
Fixes: 4342306f0f0d ("fs/ntfs3: Add file operations and implementation")
Assisted-by: LLM
Signed-off-by: Eugene Susla <eugenesusla@gmail.com>
---

Notes (not part of the changelog):

I hit this on 7.2.9 with a Windows dual-boot volume where my home folder
had NTFS compression enabled: two concurrent git commands in one repo
ended up in D state in ni_read_folio_cmpr() and the machine could no
longer suspend. A reboot was the only way out.

AI disclosure: the diagnosis, the reproducer and this patch were produced
with an LLM coding assistant, which also ran the tests listed above on my
machine. I have reviewed the analysis, the patch and the test results.

Not tested:
- concurrent writes: the reader vs ntfs_compress_write() case is
  covered by reasoning only, not by a test;
- LZX/XPRESS (WOF) compressed files, only LZNT;
- the syzbot report above (no reproducer, auto-obsoleted) shows a reader
  waiting in ntfs_lock_new_page() and a writer waiting in
  ntfs_get_frame_pages(), i.e. that reader vs writer case; I believe
  this patch fixes it but have not confirmed that.

Build: W=1 adds no warnings to fs/ntfs3/frecord.o. The reproducer (a
Python script and a guest shell script for virtme-ng, about 100 lines)
is available on request.

 fs/ntfs3/frecord.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index bead01a95..9a071b2a1 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -1893,14 +1893,30 @@ enum REPARSE_SIGN ni_parse_reparse(struct ntfs_inode *ni, struct ATTRIB *attr,
 	return REPARSE_LINK;
 }
 
+/*
+ * ntfs_lock_new_page - Get a locked page to decompress a frame into.
+ *
+ * With @nowait the page lock is only tried, never waited for: the caller
+ * already holds the lock of another page of the same frame, and a second
+ * reader doing the same for a different page of that frame would deadlock
+ * with it. A page locked by someone else is left to its owner and a
+ * temporary page is used instead.
+ */
 static struct folio *ntfs_lock_new_page(struct address_space *mapping,
-					pgoff_t index, gfp_t gfp)
+					pgoff_t index, gfp_t gfp, bool nowait)
 {
-	struct folio *folio = __filemap_get_folio(
-		mapping, index, FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
+	fgf_t fgp = FGP_LOCK | FGP_ACCESSED | FGP_CREAT;
+	struct folio *folio;
 
-	if (IS_ERR(folio))
-		return folio;
+	if (nowait)
+		fgp |= FGP_NOWAIT;
+
+	folio = __filemap_get_folio(mapping, index, fgp, gfp);
+	if (IS_ERR(folio)) {
+		if (!nowait || PTR_ERR(folio) != -EAGAIN)
+			return folio;
+		goto use_tmp;
+	}
 
 	if (!folio_test_uptodate(folio)) {
 		struct page *page = folio_file_page(folio, index);
@@ -1913,6 +1929,7 @@ static struct folio *ntfs_lock_new_page(struct address_space *mapping,
 	/* Use a temporary page to avoid data corruption */
 	folio_unlock(folio);
 	folio_put(folio);
+use_tmp:
 	folio = folio_alloc(gfp, 0);
 	if (!folio)
 		return ERR_PTR(-ENOMEM);
@@ -1975,7 +1992,7 @@ int ni_read_folio_cmpr(struct ntfs_inode *ni, struct folio *folio)
 		if (i == idx)
 			continue;
 
-		f = ntfs_lock_new_page(mapping, index, gfp_mask);
+		f = ntfs_lock_new_page(mapping, index, gfp_mask, true);
 		if (IS_ERR(f)) {
 			err = PTR_ERR(f);
 			goto out1;
@@ -2072,7 +2089,7 @@ int ni_decompress_file(struct ntfs_inode *ni)
 		for (i = 0; i < pages_per_frame; i++, index++) {
 			struct folio *f;
 
-			f = ntfs_lock_new_page(mapping, index, gfp_mask);
+			f = ntfs_lock_new_page(mapping, index, gfp_mask, false);
 			if (IS_ERR(f)) {
 				while (i--) {
 					unlock_page(pages[i]);
-- 
2.56.0