linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Compression fixes for ntfs3
@ 2025-07-18 19:53 Matthew Wilcox (Oracle)
  2025-07-18 19:53 ` [PATCH 1/3] ntfs: Do not kmap pages used for reading from disk Matthew Wilcox (Oracle)
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Matthew Wilcox (Oracle) @ 2025-07-18 19:53 UTC (permalink / raw)
  To: Konstantin Komarov; +Cc: Matthew Wilcox (Oracle), ntfs3, linux-fsdevel

Hi Konstantin,

I found three problems with the NTFS (de)compression code.  The
first two are simple inefficiencies; there's no need to kmap these
pages.  The third looks like a potential data corruption (description
in the patch).  I've marked the third one for backport.

I haven't tested any of this; it could be that my analysis is wrong.

Matthew Wilcox (Oracle) (3):
  ntfs: Do not kmap pages used for reading from disk
  ntfs: Do not kmap page cache pages for compression
  ntfs: Do not overwrite uptodate pages

 fs/ntfs3/frecord.c | 50 +++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

-- 
2.47.2


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/3] ntfs: Do not kmap pages used for reading from disk
  2025-07-18 19:53 [PATCH 0/3] Compression fixes for ntfs3 Matthew Wilcox (Oracle)
@ 2025-07-18 19:53 ` Matthew Wilcox (Oracle)
  2025-07-18 19:53 ` [PATCH 2/3] ntfs: Do not kmap page cache pages for compression Matthew Wilcox (Oracle)
  2025-07-18 19:53 ` [PATCH 3/3] ntfs: Do not overwrite uptodate pages Matthew Wilcox (Oracle)
  2 siblings, 0 replies; 4+ messages in thread
From: Matthew Wilcox (Oracle) @ 2025-07-18 19:53 UTC (permalink / raw)
  To: Konstantin Komarov; +Cc: Matthew Wilcox (Oracle), ntfs3, linux-fsdevel

These pages are accessed through DMA and vmap; they are not accessed
by calling page_address(), so they do not need to be kmapped.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/ntfs3/frecord.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index 37826288fbb0..c41968fcab00 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -2590,7 +2590,6 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages,
 		}
 		pages_disk[i] = pg;
 		lock_page(pg);
-		kmap(pg);
 	}
 
 	/* Read 'ondisk_size' bytes from disk. */
@@ -2640,7 +2639,6 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages,
 	for (i = 0; i < npages_disk; i++) {
 		pg = pages_disk[i];
 		if (pg) {
-			kunmap(pg);
 			unlock_page(pg);
 			put_page(pg);
 		}
@@ -2735,7 +2733,6 @@ int ni_write_frame(struct ntfs_inode *ni, struct page **pages,
 		}
 		pages_disk[i] = pg;
 		lock_page(pg);
-		kmap(pg);
 	}
 
 	/* To simplify compress algorithm do vmap for source and target pages. */
@@ -2823,7 +2820,6 @@ int ni_write_frame(struct ntfs_inode *ni, struct page **pages,
 	for (i = 0; i < pages_per_frame; i++) {
 		pg = pages_disk[i];
 		if (pg) {
-			kunmap(pg);
 			unlock_page(pg);
 			put_page(pg);
 		}
-- 
2.47.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/3] ntfs: Do not kmap page cache pages for compression
  2025-07-18 19:53 [PATCH 0/3] Compression fixes for ntfs3 Matthew Wilcox (Oracle)
  2025-07-18 19:53 ` [PATCH 1/3] ntfs: Do not kmap pages used for reading from disk Matthew Wilcox (Oracle)
@ 2025-07-18 19:53 ` Matthew Wilcox (Oracle)
  2025-07-18 19:53 ` [PATCH 3/3] ntfs: Do not overwrite uptodate pages Matthew Wilcox (Oracle)
  2 siblings, 0 replies; 4+ messages in thread
From: Matthew Wilcox (Oracle) @ 2025-07-18 19:53 UTC (permalink / raw)
  To: Konstantin Komarov; +Cc: Matthew Wilcox (Oracle), ntfs3, linux-fsdevel

These pages are accessed through vmap; they are not accessed
by calling page_address(), so they do not need to be kmapped.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/ntfs3/frecord.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index c41968fcab00..6fc7b2281fed 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -2407,9 +2407,6 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages,
 	 * To simplify decompress algorithm do vmap for source
 	 * and target pages.
 	 */
-	for (i = 0; i < pages_per_frame; i++)
-		kmap(pages[i]);
-
 	frame_size = pages_per_frame << PAGE_SHIFT;
 	frame_mem = vmap(pages, pages_per_frame, VM_MAP, PAGE_KERNEL);
 	if (!frame_mem) {
@@ -2655,7 +2652,6 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages,
 out:
 	for (i = 0; i < pages_per_frame; i++) {
 		pg = pages[i];
-		kunmap(pg);
 		SetPageUptodate(pg);
 	}
 
@@ -2742,9 +2738,6 @@ int ni_write_frame(struct ntfs_inode *ni, struct page **pages,
 		goto out1;
 	}
 
-	for (i = 0; i < pages_per_frame; i++)
-		kmap(pages[i]);
-
 	/* Map in-memory frame for read-only. */
 	frame_mem = vmap(pages, pages_per_frame, VM_MAP, PAGE_KERNEL_RO);
 	if (!frame_mem) {
@@ -2810,11 +2803,7 @@ int ni_write_frame(struct ntfs_inode *ni, struct page **pages,
 
 out3:
 	vunmap(frame_mem);
-
 out2:
-	for (i = 0; i < pages_per_frame; i++)
-		kunmap(pages[i]);
-
 	vunmap(frame_ondisk);
 out1:
 	for (i = 0; i < pages_per_frame; i++) {
-- 
2.47.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 3/3] ntfs: Do not overwrite uptodate pages
  2025-07-18 19:53 [PATCH 0/3] Compression fixes for ntfs3 Matthew Wilcox (Oracle)
  2025-07-18 19:53 ` [PATCH 1/3] ntfs: Do not kmap pages used for reading from disk Matthew Wilcox (Oracle)
  2025-07-18 19:53 ` [PATCH 2/3] ntfs: Do not kmap page cache pages for compression Matthew Wilcox (Oracle)
@ 2025-07-18 19:53 ` Matthew Wilcox (Oracle)
  2 siblings, 0 replies; 4+ messages in thread
From: Matthew Wilcox (Oracle) @ 2025-07-18 19:53 UTC (permalink / raw)
  To: Konstantin Komarov; +Cc: Matthew Wilcox (Oracle), ntfs3, linux-fsdevel, stable

When reading a compressed file, we may read several pages in addition to
the one requested.  The current code will overwrite pages in the page
cache with the data from disc which can definitely result in changes
that have been made being lost.

For example if we have four consecutie pages ABCD in the file compressed
into a single extent, on first access, we'll bring in ABCD.  Then we
write to page B.  Memory pressure results in the eviction of ACD.
When we attempt to write to page C, we will overwrite the data in page
B with the data currently on disk.

I haven't investigated the decompression code to check whether it's
OK to overwrite a clean page or whether it might be possible to see
corrupt data.  Out of an abundance of caution, decline to overwrite
uptodate pages, not just dirty pages.

Fixes: 4342306f0f0d (fs/ntfs3: Add file operations and implementation)
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: stable@vger.kernel.org
---
 fs/ntfs3/frecord.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index 6fc7b2281fed..c3ce9cf4441e 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -2020,6 +2020,29 @@ int ni_fiemap(struct ntfs_inode *ni, struct fiemap_extent_info *fieinfo,
 	return err;
 }
 
+static struct page *ntfs_lock_new_page(struct address_space *mapping,
+		pgoff_t index, gfp_t gfp)
+{
+	struct folio *folio = __filemap_get_folio(mapping, index,
+			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
+	struct page *page;
+
+	if (IS_ERR(folio))
+		return ERR_CAST(folio);
+
+	if (!folio_test_uptodate(folio))
+		return folio_file_page(folio, index);
+
+	/* Use a temporary page to avoid data corruption */
+	folio_unlock(folio);
+	folio_put(folio);
+	page = alloc_page(gfp);
+	if (!page)
+		return ERR_PTR(-ENOMEM);
+	__SetPageLocked(page);
+	return page;
+}
+
 /*
  * ni_readpage_cmpr
  *
@@ -2074,9 +2097,9 @@ int ni_readpage_cmpr(struct ntfs_inode *ni, struct folio *folio)
 		if (i == idx)
 			continue;
 
-		pg = find_or_create_page(mapping, index, gfp_mask);
-		if (!pg) {
-			err = -ENOMEM;
+		pg = ntfs_lock_new_page(mapping, index, gfp_mask);
+		if (IS_ERR(pg)) {
+			err = PTR_ERR(pg);
 			goto out1;
 		}
 		pages[i] = pg;
@@ -2175,13 +2198,13 @@ int ni_decompress_file(struct ntfs_inode *ni)
 		for (i = 0; i < pages_per_frame; i++, index++) {
 			struct page *pg;
 
-			pg = find_or_create_page(mapping, index, gfp_mask);
-			if (!pg) {
+			pg = ntfs_lock_new_page(mapping, index, gfp_mask);
+			if (IS_ERR(pg)) {
 				while (i--) {
 					unlock_page(pages[i]);
 					put_page(pages[i]);
 				}
-				err = -ENOMEM;
+				err = PTR_ERR(pg);
 				goto out;
 			}
 			pages[i] = pg;
-- 
2.47.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-07-18 19:54 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-18 19:53 [PATCH 0/3] Compression fixes for ntfs3 Matthew Wilcox (Oracle)
2025-07-18 19:53 ` [PATCH 1/3] ntfs: Do not kmap pages used for reading from disk Matthew Wilcox (Oracle)
2025-07-18 19:53 ` [PATCH 2/3] ntfs: Do not kmap page cache pages for compression Matthew Wilcox (Oracle)
2025-07-18 19:53 ` [PATCH 3/3] ntfs: Do not overwrite uptodate pages Matthew Wilcox (Oracle)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).