[PATCH] ubifs: zero stale data beyond EOF before file size grows

[PATCH] ubifs: zero stale data beyond EOF before file size grows

[Nan Lin]

mmap writes can dirty the page cache beyond EOF without changing
i_size. A later extending write or ftruncate turns that region into a
hole, but UBIFS never zeroes the partial page at the old EOF, so reads
expose the stale data.

Zero the tail of the old-EOF folio in all three size-extending paths:
ubifs_write_begin() for writes starting beyond EOF, do_setattr()
before truncate_setsize() grows the size, and ubifs_vm_page_mkwrite()
so mmap writes cannot leave such a tail behind. This mirrors what
block_page_mkwrite() does for buffer_head filesystems and what ext4
does on size extension.

All zeroing takes the old-EOF folio lock without ui->ui_mutex, keeping
the folio_lock -> ui_mutex order of ->writepage() and
->page_mkwrite(); the reverse order deadlocks.

Signed-off-by: Nan Lin <lin.nan@h3c.com>
---
 fs/ubifs/file.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index e73c28b12f97..e09b17d36495 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -202,6 +202,53 @@ static void release_existing_page_budget(struct ubifs_info *c)
 	ubifs_release_budget(c, &req);
 }
 
+/**
+ * ubifs_zero_eof_hole - zero the stale tail of the old-EOF folio.
+ * @inode: inode whose size is about to grow
+ * @new_end: new end of the file, only used to bound the tail
+ *
+ * Zero the cached folio containing the old EOF for [old EOF,
+ * min(folio end, @new_end)) so a size extension does not expose stale
+ * data left beyond EOF by mmap writes.
+ *
+ * Must be called without ui->ui_mutex held: we block on the old-EOF
+ * folio lock, and ->writepage() and ->page_mkwrite() hold that folio
+ * lock while taking ui_mutex; the reverse order deadlocks.
+ */
+static void ubifs_zero_eof_hole(struct inode *inode, loff_t new_end)
+{
+	loff_t old_size = i_size_read(inode);
+	unsigned int from, to;
+	struct folio *folio;
+
+	if (new_end <= old_size)
+		return;
+
+	folio = filemap_lock_folio(inode->i_mapping, old_size >> PAGE_SHIFT);
+	if (IS_ERR(folio))
+		return;
+
+	/*
+	 * Re-read i_size under the folio lock: a concurrent truncate may
+	 * have shrunk the file, turning this folio into valid data.
+	 */
+	old_size = i_size_read(inode);
+	if (new_end <= old_size)
+		goto out;
+
+	from = old_size & (PAGE_SIZE - 1);
+	if (!from)
+		goto out;
+	to = (old_size >> PAGE_SHIFT) == (new_end >> PAGE_SHIFT) ?
+	     new_end & (PAGE_SIZE - 1) : PAGE_SIZE;
+	if (to > from)
+		folio_zero_range(folio, from, to - from);
+
+out:
+	folio_unlock(folio);
+	folio_put(folio);
+}
+
 static int write_begin_slow(struct address_space *mapping,
 			    loff_t pos, unsigned len, struct folio **foliop)
 {
@@ -423,6 +470,14 @@ static int ubifs_write_begin(const struct kiocb *iocb,
 	if (unlikely(c->ro_error))
 		return -EROFS;
 
+	/*
+	 * The write starts beyond EOF: zero the stale tail of the old-EOF
+	 * folio before budgeting takes ui->ui_mutex. do_readpage() only
+	 * sanitizes the write folio, not the old-EOF folio.
+	 */
+	if (pos > inode->i_size)
+		ubifs_zero_eof_hole(inode, pos);
+
 	/* Try out the fast-path part first */
 	folio = __filemap_get_folio(mapping, index, FGP_WRITEBEGIN,
 			mapping_gfp_mask(mapping));
@@ -1220,6 +1275,10 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode,
 
 	if (attr->ia_valid & ATTR_SIZE) {
 		dbg_gen("size %lld -> %lld", inode->i_size, new_size);
+
+		/* Must run before the size change loses the old boundary. */
+		ubifs_zero_eof_hole(inode, new_size);
+
 		truncate_setsize(inode, new_size);
 	}
 
@@ -1494,6 +1553,7 @@ static vm_fault_t ubifs_vm_page_mkwrite(struct vm_fault *vmf)
 	struct timespec64 now = current_time(inode);
 	struct ubifs_budget_req req = { .new_page = 1 };
 	int err, update_time;
+	loff_t i_size;
 
 	dbg_gen("ino %llu, pg %lu, i_size %lld",	inode->i_ino, folio->index,
 		i_size_read(inode));
@@ -1553,6 +1613,19 @@ static vm_fault_t ubifs_vm_page_mkwrite(struct vm_fault *vmf)
 		filemap_dirty_folio(folio->mapping, folio);
 	}
 
+	/*
+	 * Zero the part of the EOF folio beyond i_size so mmap writes
+	 * cannot leave a stale tail behind (see ubifs_zero_eof_hole()).
+	 * For a page-aligned i_size the mkwrite entry check has already
+	 * returned SIGBUS, so @from is always non-zero here.
+	 */
+	i_size = i_size_read(inode);
+	if (folio->index == (i_size >> PAGE_SHIFT)) {
+		unsigned int from = i_size & (PAGE_SIZE - 1);
+
+		folio_zero_range(folio, from, PAGE_SIZE - from);
+	}
+
 	if (update_time) {
 		int release;
 		struct ubifs_inode *ui = ubifs_inode(inode);
-- 
2.33.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] ubifs: zero stale data beyond EOF before file size grows

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ubifs: data corruption when extending files due to page-based arithmetic on large folios
--

commit 979d29c432b12d088592388772df15df41ba2373
Author: Nan Lin <lin.nan@h3c.com>

ubifs: zero stale data beyond EOF before file size grows

This commit zeroes the tail of the old-EOF folio before file size grows
to prevent exposing stale data from mmap writes. It handles writes
starting beyond EOF, truncation size growth, and page_mkwrite operations.

> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index e73c28b12f97f..e09b17d364952 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
[ ... ]
> @@ -202,6 +202,53 @@ static void release_existing_page_budget(struct ubifs_info *c)
>  	ubifs_release_budget(c, &req);
>  }
>  
> +/**
> + * ubifs_zero_eof_hole - zero the stale tail of the old-EOF folio.
> + * @inode: inode whose size is about to grow
> + * @new_end: new end of the file, only used to bound the tail
> + *
> + * Zero the cached folio containing the old EOF for [old EOF,
> + * min(folio end, @new_end)) so a size extension does not expose stale
> + * data left beyond EOF by mmap writes.
> + *
> + * Must be called without ui->ui_mutex held: we block on the old-EOF
> + * folio lock, and ->writepage() and ->page_mkwrite() hold that folio
> + * lock while taking ui_mutex; the reverse order deadlocks.
> + */
> +static void ubifs_zero_eof_hole(struct inode *inode, loff_t new_end)
> +{
[ ... ]
> +	from = old_size & (PAGE_SIZE - 1);
> +	if (!from)
> +		goto out;
> +	to = (old_size >> PAGE_SHIFT) == (new_end >> PAGE_SHIFT) ?
> +	     new_end & (PAGE_SIZE - 1) : PAGE_SIZE;
> +	if (to > from)
> +		folio_zero_range(folio, from, to - from);

[Severity: High]
Does this use of page-relative arithmetic with folio_zero_range() cause
data corruption when dealing with large folios?

The offset 'from' is calculated as 'old_size & (PAGE_SIZE - 1)'. If 'old_size'
is located beyond the first page of a large folio (e.g., at byte 4097), 'from'
evaluates to an offset within the sub-page (1).

However, folio_zero_range() expects a byte offset relative to the start of
the entire folio. Passing 1 here would cause it to zero starting from byte 1
of the folio (and thus the file), corrupting valid file data. Furthermore,
'to' is capped at PAGE_SIZE, which would fail to zero the rest of the hole if
it spans multiple pages within the same large folio.

Should the calculation use offset_in_folio() and folio_size() instead?

> +
> +out:
> +	folio_unlock(folio);
> +	folio_put(folio);
> +}
[ ... ]
> @@ -1553,6 +1613,19 @@ static vm_fault_t ubifs_vm_page_mkwrite(struct vm_fault *vmf)
>  		filemap_dirty_folio(folio->mapping, folio);
>  	}
>  
> +	/*
> +	 * Zero the part of the EOF folio beyond i_size so mmap writes
> +	 * cannot leave a stale tail behind (see ubifs_zero_eof_hole()).
> +	 * For a page-aligned i_size the mkwrite entry check has already
> +	 * returned SIGBUS, so @from is always non-zero here.
> +	 */
> +	i_size = i_size_read(inode);
> +	if (folio->index == (i_size >> PAGE_SHIFT)) {
> +		unsigned int from = i_size & (PAGE_SIZE - 1);
> +
> +		folio_zero_range(folio, from, PAGE_SIZE - from);
> +	}

[Severity: High]
Does the condition 'folio->index == (i_size >> PAGE_SHIFT)' evaluate to false
for the second or later page of a large folio?

If so, it would skip tail zeroing entirely when the EOF is located in a large
folio but not within its first page. The calculation for 'from' here also seems
to apply incorrect page-relative sizes rather than folio-relative.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016090458.1891272-1-lin.nan@h3c.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/