[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/

[PATCH v2] 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.

UBIFS does not enable large folios, so the EOF folio is always a
single page today; nevertheless, use folio-relative arithmetic
(offset_in_folio(), folio_contains(), folio_size()) throughout so the
code is correct if that ever changes.

Signed-off-by: Nan Lin <lin.nan@h3c.com>

Changes since v1:
 - Both zeroing sites used PAGE_SIZE-relative arithmetic
   (offset_in_page()-style) while folio_zero_range() expects
   folio-relative offsets. UBIFS does not currently enable large
   folios, so PAGE_SIZE == folio_size() and the two coincide in
   practice, but switch to offset_in_folio()/folio_size() anyway so
   the code is correct if UBIFS ever opts into large folios
   (suggested by Sashiko AI review).

---
 fs/ubifs/file.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index e73c28b12f97..1726714161a1 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);
+	size_t 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 = offset_in_folio(folio, old_size);
+	if (!from)
+		goto out;
+	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
+	     offset_in_folio(folio, new_end) : folio_size(folio);
+	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 folio-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_contains(folio, i_size >> PAGE_SHIFT)) {
+		size_t from = offset_in_folio(folio, i_size);
+
+		folio_zero_range(folio, from, folio_size(folio) - 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 v2] ubifs: zero stale data beyond EOF before file size grows

[Zhihao Cheng]

在 2026/10/19 11:25, Nan Lin 写道:
Hi 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.
> 
> UBIFS does not enable large folios, so the EOF folio is always a
> single page today; nevertheless, use folio-relative arithmetic
> (offset_in_folio(), folio_contains(), folio_size()) throughout so the
> code is correct if that ever changes.
> 
> Signed-off-by: Nan Lin <lin.nan@h3c.com>
> 
> Changes since v1:
>   - Both zeroing sites used PAGE_SIZE-relative arithmetic
>     (offset_in_page()-style) while folio_zero_range() expects
>     folio-relative offsets. UBIFS does not currently enable large
>     folios, so PAGE_SIZE == folio_size() and the two coincide in
>     practice, but switch to offset_in_folio()/folio_size() anyway so
>     the code is correct if UBIFS ever opts into large folios
>     (suggested by Sashiko AI review).
> 
> ---
>   fs/ubifs/file.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 73 insertions(+)
> 
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index e73c28b12f97..1726714161a1 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);
> +	size_t 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;
The filemap_lock_folio gets folio with FGP_LOCK, I think we should use 
'FGP_WRITEBEGIN | FGP_ACCESSED' just like ext4_load_tail_bh() does, 
because the folio may not exist in ram when calling do_setattr().
> +
> +	/*
> +	 * 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 = offset_in_folio(folio, old_size);
> +	if (!from)
> +		goto out;
Why skipping 'from == 0'? What if a user writes begin from 
offset-X(0<X<4096) to a new file(isize = 0)? Ext4 zero the range by 
ext4_block_write_begin->folio_zero_segments(folio, to, block_end, 
**block_start**, **from**), but ubifs not handle it.
> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
> +	     offset_in_folio(folio, new_end) : folio_size(folio);
> +	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);
> +

The ext4 protection check is ext4_write_checks->ext4_block_zero_eof, I 
think we can add the zero hole check in ubifs_write_iter().
>   	/* 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 folio-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_contains(folio, i_size >> PAGE_SHIFT)) {
> +		size_t from = offset_in_folio(folio, i_size);
> +
If the program enters into this branch, the ubifs file's isize is not 
zero, ubifs_read_folio has already zerod tail, do we still need this 
folio_zero_range?
> +		folio_zero_range(folio, from, folio_size(folio) - from);
> +	}
> +
>   	if (update_time) {
>   		int release;
>   		struct ubifs_inode *ui = ubifs_inode(inode);
> 


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

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

[Zhihao Cheng]

在 2026/10/19 20:38, Zhihao Cheng 写道:
> 在 2026/10/19 11:25, Nan Lin 写道:
> Hi 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.
>>
>> UBIFS does not enable large folios, so the EOF folio is always a
>> single page today; nevertheless, use folio-relative arithmetic
>> (offset_in_folio(), folio_contains(), folio_size()) throughout so the
>> code is correct if that ever changes.
>>
>> Signed-off-by: Nan Lin <lin.nan@h3c.com>
>>
>> Changes since v1:
>>   - Both zeroing sites used PAGE_SIZE-relative arithmetic
>>     (offset_in_page()-style) while folio_zero_range() expects
>>     folio-relative offsets. UBIFS does not currently enable large
>>     folios, so PAGE_SIZE == folio_size() and the two coincide in
>>     practice, but switch to offset_in_folio()/folio_size() anyway so
>>     the code is correct if UBIFS ever opts into large folios
>>     (suggested by Sashiko AI review).
>>
>> ---
>>   fs/ubifs/file.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index e73c28b12f97..1726714161a1 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);
>> +    size_t 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;
> The filemap_lock_folio gets folio with FGP_LOCK, I think we should use 
> 'FGP_WRITEBEGIN | FGP_ACCESSED' just like ext4_load_tail_bh() does, 
> because the folio may not exist in ram when calling do_setattr().
>> +
>> +    /*
>> +     * 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 = offset_in_folio(folio, old_size);
>> +    if (!from)
>> +        goto out;
> Why skipping 'from == 0'? What if a user writes begin from 
> offset-X(0<X<4096) to a new file(isize = 0)? Ext4 zero the range by 
> ext4_block_write_begin->folio_zero_segments(folio, to, block_end, 
> **block_start**, **from**), but ubifs not handle it.
>> +    to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
>> +         offset_in_folio(folio, new_end) : folio_size(folio);
>> +    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);
>> +
> 
> The ext4 protection check is ext4_write_checks->ext4_block_zero_eof, I 
> think we can add the zero hole check in ubifs_write_iter().
>>       /* 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 folio-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_contains(folio, i_size >> PAGE_SHIFT)) {
>> +        size_t from = offset_in_folio(folio, i_size);
>> +
> If the program enters into this branch, the ubifs file's isize is not 
> zero, ubifs_read_folio has already zerod tail, do we still need this 
> folio_zero_range?
Oh, there is one case, UBIFS file has isize without data nodes(eg. isize 
= 3, the first block is a data hole), ubifs_read_folio won't zero the 
page tail, we still need folio_zero_range().
>> +        folio_zero_range(folio, from, folio_size(folio) - from);
>> +    }
>> +
>>       if (update_time) {
>>           int release;
>>           struct ubifs_inode *ui = ubifs_inode(inode);
>>
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


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

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

[Nan Lin]

Hi Cheng,

Thanks for the detailed review. My responses are inline.

> The filemap_lock_folio gets folio with FGP_LOCK, I think we should use
> 'FGP_WRITEBEGIN | FGP_ACCESSED' just like ext4_load_tail_bh() does,
> because the folio may not exist in ram when calling do_setattr().

ext4_load_tail_bh() needs FGP_CREAT because in Ext4 the block behind an
unwritten/delayed extent holds garbage on disk, so the zeroing must
reach the media. UBIFS does not have this problem -- stale data can
only live in the page cache, never on flash:

  writeback:  ubifs_writepage() zeroes the straddling folio on every
              invocation and skips folios fully beyond i_size, so
              stale tails never reach the media.
  read-in:    do_readpage()/read_block() zero the tails of holes and
              short data nodes, so a folio loaded from flash is always
              tail-clean.

So when the EOF folio is absent from the cache, there is nothing to
zero anywhere: FGP_CREAT would merely allocate a fresh (already zero)
folio to zero it again. Returning early on IS_ERR() is the correct and
cheaper equivalent of Ext4's path.

> Why skipping 'from == 0'? What if a user writes begin from
> offset-X(0<X<4096) to a new file(isize = 0)? Ext4 zero the range by
> ext4_block_write_begin->folio_zero_segments(folio, to, block_end,
> block_start, from), but ubifs not handle it.

from == 0 means the old EOF is folio-aligned, so the EOF folio has no
bytes beyond EOF and there is no hole in it:

  from != 0:  |/////valid/////|stale|   <- zero [from, to)
  from == 0:  |/////valid/////|         <- nothing beyond EOF here

For the i_size == 0 case: a zero-size file cannot hold a polluted
folio. mmap cannot create one (page_mkwrite returns SIGBUS when
folio_pos >= i_size), and write() followed by ftruncate(0) invalidates
the folio. The [0, X) part of the write folio itself is zeroed by the
existing do_readpage() path ("block >= beyond" ->
folio_zero_range()), which is UBIFS's counterpart of
ext4_block_write_begin()'s folio_zero_segments() -- the work happens
at folio read-in because UBIFS's block size always equals the page
size.

> The ext4 protection check is ext4_write_checks->ext4_block_zero_eof,
> I think we can add the zero hole check in ubifs_write_iter().

Agreed. This mirrors ext4_write_checks() -> ext4_block_zero_eof() and
runs once per syscall instead of once per write_begin() iteration.
Will move it there in v3. 

> If the program enters into this branch, the ubifs file's isize is not
> zero, ubifs_read_folio has already zerod tail, do we still need this
> folio_zero_range?
> Oh, there is one case, UBIFS file has isize without data nodes(eg.
> isize = 3, the first block is a data hole), ubifs_read_folio won't
> zero the page tail, we still need folio_zero_range().

Right -- and one more case on top of the hole-inode one: mkwrite also
runs on already-cached folios, and earlier mmap stores may have
re-dirtied the tail after ubifs_read_folio() cleaned it (only folios
fully beyond EOF get SIGBUS; the straddling folio stays writable
across its whole length). So the tail must be re-zeroed on every
(re-)fault, and the folio_zero_range() stays.

For reproducibility I have attached a small single-writer reproducer.
It covers both size-extending paths (S1: pwrite beyond EOF ->
write_begin/write_end, S2: ftruncate up -> do_setattr) against a
polluted old-EOF folio. 

Build: gcc -O2 -Wall -o eof_hole_test eof_hole_test.c
Run:   ./eof_hole_test <mountpoint>/eof_hole.bin

-----------------[ eof_hole_test.c ]-----------------
/*
 * eof_hole_repro.c -- repro for "stale data in hole" bug
 *
 * Reproduce:
 *   1. pwrite data to [0, 0x2000)          - full page 0 and 1
 *   2. ftruncate DOWN to 0x1100            - VFS zeroes page-1 tail
 *   3. mmap page 1; write junk into [0x1100, 0x1200) via the mapping
 *      (MAP_SHARED, past EOF)              - pollutes the cache tail,
 *                                            i_size unchanged, legal
 *   4. pwrite at 0x3000 (beyond EOF)       - i_size jumps; UBIFS's
 *                                            ubifs_write_end() does NOT
 *                                            zero [0x1100, 0x3000)
 *   5. pread [0x1100, 0x3000): must be all zero.
 *      Non-zero bytes = stale mmap junk exposed = bug reproduced.
 *
 * Scenario 2 uses ftruncate UP instead of pwrite to exercise
 * do_setattr() instead of write_end().
 *
 * Build: gcc -O2 -Wall -o eof_hole_test eof_hole_test.c
 * Run:   ./eof_hole_test /mnt/flash/eof_hole.bin
 */
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/mman.h>

#define PS       4096
#define OLD_EOF  (PS + 0x100)   /* 0x1100: old EOF mid-page-1 */
#define NEW_END  (3 * PS)       /* 0x3000: hole end */
#define JUNK_LEN 0x100          /* 256 bytes of pollution */

static int check_hole(int fd, off_t off, size_t len, const char *what)
{
	unsigned char buf[512];
	size_t n, i;

	while (len) {
		n = len > sizeof(buf) ? sizeof(buf) : len;
		if (pread(fd, buf, n, off) != (ssize_t)n) {
			printf("FAIL: %s: pread error: %s\n", what,
			       strerror(errno));
			return 1;
		}
		for (i = 0; i < n; i++) {
			if (buf[i] != 0) {
				printf("FAIL: %s: byte 0x%02x at offset 0x%llx "
				       "(hole offset +%zu)\n",
				       what, buf[i],
				       (unsigned long long)(off + i), i);
				return 1;
			}
		}
		off += n;
		len -= n;
	}
	printf("PASS: %s\n", what);
	return 0;
}

/* Create a file with EOF mid-page-1, then pollute the page-cache tail
 * beyond EOF via an mmap write (the only path that skips zeroing). */
static int make_file(const char *path)
{
	unsigned char junk[PS];
	void *map;
	int fd;

	memset(junk, 0x5a, sizeof(junk));

	fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0644);
	if (fd < 0) {
		perror("open");
		return -1;
	}

	/* page 1 full of junk, durable */
	if (pwrite(fd, junk, PS, PS) != PS) {
		perror("pwrite");
		goto bad;
	}
	fsync(fd);

	/* shrink: VFS zeroes page-1 tail [OLD_EOF, 0x2000) itself */
	if (ftruncate(fd, OLD_EOF)) {
		perror("ftruncate");
		goto bad;
	}

	/* re-pollute [OLD_EOF, OLD_EOF+JUNK_LEN) through a shared
	 * mapping. This goes through ->page_mkwrite() which does no
	 * zeroing. */
	map = mmap(NULL, PS, PROT_READ | PROT_WRITE, MAP_SHARED, fd, PS);
	if (map == MAP_FAILED) {
		perror("mmap");
		goto bad;
	}
	memcpy((char *)map + 0x100, junk, JUNK_LEN);
	munmap(map, PS);
	return fd;

bad:
	close(fd);
	return -1;
}

int main(int argc, char **argv)
{
	const char *path = argc > 1 ? argv[1] : "./eof_hole.bin";
	unsigned char junk[JUNK_LEN];
	int fd, fail = 0;

	memset(junk, 0x5a, sizeof(junk));

	/* ---------- S1: buffered write beyond EOF ----------
	 * exercises ubifs_write_end() appending branch            */
	fd = make_file(path);
	if (fd < 0)
		return 1;
	if (pwrite(fd, junk, JUNK_LEN, NEW_END) != JUNK_LEN) {
		perror("pwrite");
		return 1;
	}
	fsync(fd);
	fail |= check_hole(fd, OLD_EOF, NEW_END - OLD_EOF,
			   "S1 write_end: hole [0x1100,0x3000)");
	close(fd);

	/* ---------- S2: ftruncate UP ----------
	 * exercises do_setattr() ATTR_SIZE branch                  */
	fd = make_file(path);
	if (fd < 0)
		return 1;
	if (ftruncate(fd, NEW_END)) {
		perror("ftruncate");
		return 1;
	}
	fail |= check_hole(fd, OLD_EOF, NEW_END - OLD_EOF,
			   "S2 do_setattr: hole [0x1100,0x3000)");
	close(fd);

	unlink(path);

	if (fail) {
		printf("\n===> BUG REPRODUCED: stale data visible in hole\n");
		return 1;
	}
	printf("\n===> OK: hole reads as zeros on both paths\n");
	return 0;
}
------------------------------------------------------

--
Nan Lin

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

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

[Zhihao Cheng]

在 2026/10/22 11:18, Nan Lin 写道:
> Hi Cheng,
> 
> Thanks for the detailed review. My responses are inline.
> 
>> The filemap_lock_folio gets folio with FGP_LOCK, I think we should use
>> 'FGP_WRITEBEGIN | FGP_ACCESSED' just like ext4_load_tail_bh() does,
>> because the folio may not exist in ram when calling do_setattr().
> 
> ext4_load_tail_bh() needs FGP_CREAT because in Ext4 the block behind an
> unwritten/delayed extent holds garbage on disk, so the zeroing must
> reach the media. UBIFS does not have this problem -- stale data can
> only live in the page cache, never on flash:
> 
>    writeback:  ubifs_writepage() zeroes the straddling folio on every
>                invocation and skips folios fully beyond i_size, so
>                stale tails never reach the media.
>    read-in:    do_readpage()/read_block() zero the tails of holes and
>                short data nodes, so a folio loaded from flash is always
>                tail-clean.
> 
> So when the EOF folio is absent from the cache, there is nothing to
> zero anywhere: FGP_CREAT would merely allocate a fresh (already zero)
> folio to zero it again. Returning early on IS_ERR() is the correct and
> cheaper equivalent of Ext4's path.

There is one case makes me confused:
File f, isize = 0
1. setattr(f, isize = 3)
    do_setattr -> ubifs_zero_eof_hole -> filemap_lock_folio // gets no 
folio and returns, nothing happens
2. pwrite(f, "123", offset = 6, len = 3)
    ubifs_write_begin
     ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and 
returns, nothing happens
     folio = __filemap_get_folio(FGP_WRITEBEGIN) // create a new folio 
with random content
     if (!folio_test_uptodate(folio))
       do_readpage // does nothing
        while (1) {
          ENOENT = read_block
        }
    ubifs_write_end
     isize = 9

   ubifs_writepage
    folio content looks like
   | random | random | 123   |   zero       |
   0        3        6       9             4096

According to the current implementations, could above process happen?
If it is, I think we need to do two points:
1. The folio(even not cached) zero range is necessary when setting a new 
bigger size?       -- For 0~3 case
2. The folio(even not cached) zero range is necessary when writting 
starts after old isize?       -- For 3~6 case
I think zeroing the whole folio in ubifs_read_folio(ENOENT case) is a 
simple solution?
> 
>> Why skipping 'from == 0'? What if a user writes begin from
>> offset-X(0<X<4096) to a new file(isize = 0)? Ext4 zero the range by
>> ext4_block_write_begin->folio_zero_segments(folio, to, block_end,
>> block_start, from), but ubifs not handle it.
> 
> from == 0 means the old EOF is folio-aligned, so the EOF folio has no
> bytes beyond EOF and there is no hole in it:
> 
>    from != 0:  |/////valid/////|stale|   <- zero [from, to)
>    from == 0:  |/////valid/////|         <- nothing beyond EOF here
> 
> For the i_size == 0 case: a zero-size file cannot hold a polluted
> folio. mmap cannot create one (page_mkwrite returns SIGBUS when
> folio_pos >= i_size), and write() followed by ftruncate(0) invalidates
> the folio. The [0, X) part of the write folio itself is zeroed by the
> existing do_readpage() path ("block >= beyond" ->
> folio_zero_range()), which is UBIFS's counterpart of
> ext4_block_write_begin()'s folio_zero_segments() -- the work happens
> at folio read-in because UBIFS's block size always equals the page
> size.
> 

This question is similar with the previous one.

>> The ext4 protection check is ext4_write_checks->ext4_block_zero_eof,
>> I think we can add the zero hole check in ubifs_write_iter().
> 
> Agreed. This mirrors ext4_write_checks() -> ext4_block_zero_eof() and
> runs once per syscall instead of once per write_begin() iteration.
> Will move it there in v3.
> 
>> If the program enters into this branch, the ubifs file's isize is not
>> zero, ubifs_read_folio has already zerod tail, do we still need this
>> folio_zero_range?
>> Oh, there is one case, UBIFS file has isize without data nodes(eg.
>> isize = 3, the first block is a data hole), ubifs_read_folio won't
>> zero the page tail, we still need folio_zero_range().
> 
> Right -- and one more case on top of the hole-inode one: mkwrite also
> runs on already-cached folios, and earlier mmap stores may have
> re-dirtied the tail after ubifs_read_folio() cleaned it (only folios
> fully beyond EOF get SIGBUS; the straddling folio stays writable
> across its whole length). So the tail must be re-zeroed on every
> (re-)fault, and the folio_zero_range() stays.

I;m still confusing about the necessary of this folio_zero_range().
For a hole inode(isize = 3, no data nodes), if the folio is cached, 
ubifs_read_folio has zerod the whole folio content(Suspend we have 
applied the simple solution from the previous question). The subsequent 
dirty setting will not write the page content after the 3rd byte to a 
data node.
> 
> For reproducibility I have attached a small single-writer reproducer.
> It covers both size-extending paths (S1: pwrite beyond EOF ->
> write_begin/write_end, S2: ftruncate up -> do_setattr) against a
> polluted old-EOF folio.

Thanks for providing that.
> 
> Build: gcc -O2 -Wall -o eof_hole_test eof_hole_test.c
> Run:   ./eof_hole_test <mountpoint>/eof_hole.bin
> 
> -----------------[ eof_hole_test.c ]-----------------
> /*
>   * eof_hole_repro.c -- repro for "stale data in hole" bug
>   *
>   * Reproduce:
>   *   1. pwrite data to [0, 0x2000)          - full page 0 and 1
>   *   2. ftruncate DOWN to 0x1100            - VFS zeroes page-1 tail
>   *   3. mmap page 1; write junk into [0x1100, 0x1200) via the mapping
>   *      (MAP_SHARED, past EOF)              - pollutes the cache tail,
>   *                                            i_size unchanged, legal
>   *   4. pwrite at 0x3000 (beyond EOF)       - i_size jumps; UBIFS's
>   *                                            ubifs_write_end() does NOT
>   *                                            zero [0x1100, 0x3000)
>   *   5. pread [0x1100, 0x3000): must be all zero.
>   *      Non-zero bytes = stale mmap junk exposed = bug reproduced.
>   *
>   * Scenario 2 uses ftruncate UP instead of pwrite to exercise
>   * do_setattr() instead of write_end().
>   *
>   * Build: gcc -O2 -Wall -o eof_hole_test eof_hole_test.c
>   * Run:   ./eof_hole_test /mnt/flash/eof_hole.bin
>   */
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
> #include <sys/mman.h>
> 
> #define PS       4096
> #define OLD_EOF  (PS + 0x100)   /* 0x1100: old EOF mid-page-1 */
> #define NEW_END  (3 * PS)       /* 0x3000: hole end */
> #define JUNK_LEN 0x100          /* 256 bytes of pollution */
> 
> static int check_hole(int fd, off_t off, size_t len, const char *what)
> {
> 	unsigned char buf[512];
> 	size_t n, i;
> 
> 	while (len) {
> 		n = len > sizeof(buf) ? sizeof(buf) : len;
> 		if (pread(fd, buf, n, off) != (ssize_t)n) {
> 			printf("FAIL: %s: pread error: %s\n", what,
> 			       strerror(errno));
> 			return 1;
> 		}
> 		for (i = 0; i < n; i++) {
> 			if (buf[i] != 0) {
> 				printf("FAIL: %s: byte 0x%02x at offset 0x%llx "
> 				       "(hole offset +%zu)\n",
> 				       what, buf[i],
> 				       (unsigned long long)(off + i), i);
> 				return 1;
> 			}
> 		}
> 		off += n;
> 		len -= n;
> 	}
> 	printf("PASS: %s\n", what);
> 	return 0;
> }
> 
> /* Create a file with EOF mid-page-1, then pollute the page-cache tail
>   * beyond EOF via an mmap write (the only path that skips zeroing). */
> static int make_file(const char *path)
> {
> 	unsigned char junk[PS];
> 	void *map;
> 	int fd;
> 
> 	memset(junk, 0x5a, sizeof(junk));
> 
> 	fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0644);
> 	if (fd < 0) {
> 		perror("open");
> 		return -1;
> 	}
> 
> 	/* page 1 full of junk, durable */
> 	if (pwrite(fd, junk, PS, PS) != PS) {
> 		perror("pwrite");
> 		goto bad;
> 	}
> 	fsync(fd);
> 
> 	/* shrink: VFS zeroes page-1 tail [OLD_EOF, 0x2000) itself */
> 	if (ftruncate(fd, OLD_EOF)) {
> 		perror("ftruncate");
> 		goto bad;
> 	}
> 
> 	/* re-pollute [OLD_EOF, OLD_EOF+JUNK_LEN) through a shared
> 	 * mapping. This goes through ->page_mkwrite() which does no
> 	 * zeroing. */
> 	map = mmap(NULL, PS, PROT_READ | PROT_WRITE, MAP_SHARED, fd, PS);
> 	if (map == MAP_FAILED) {
> 		perror("mmap");
> 		goto bad;
> 	}
> 	memcpy((char *)map + 0x100, junk, JUNK_LEN);
> 	munmap(map, PS);
> 	return fd;
> 
> bad:
> 	close(fd);
> 	return -1;
> }
> 
> int main(int argc, char **argv)
> {
> 	const char *path = argc > 1 ? argv[1] : "./eof_hole.bin";
> 	unsigned char junk[JUNK_LEN];
> 	int fd, fail = 0;
> 
> 	memset(junk, 0x5a, sizeof(junk));
> 
> 	/* ---------- S1: buffered write beyond EOF ----------
> 	 * exercises ubifs_write_end() appending branch            */
> 	fd = make_file(path);
> 	if (fd < 0)
> 		return 1;
> 	if (pwrite(fd, junk, JUNK_LEN, NEW_END) != JUNK_LEN) {
> 		perror("pwrite");
> 		return 1;
> 	}
> 	fsync(fd);
> 	fail |= check_hole(fd, OLD_EOF, NEW_END - OLD_EOF,
> 			   "S1 write_end: hole [0x1100,0x3000)");
> 	close(fd);
> 
> 	/* ---------- S2: ftruncate UP ----------
> 	 * exercises do_setattr() ATTR_SIZE branch                  */
> 	fd = make_file(path);
> 	if (fd < 0)
> 		return 1;
> 	if (ftruncate(fd, NEW_END)) {
> 		perror("ftruncate");
> 		return 1;
> 	}
> 	fail |= check_hole(fd, OLD_EOF, NEW_END - OLD_EOF,
> 			   "S2 do_setattr: hole [0x1100,0x3000)");
> 	close(fd);
> 
> 	unlink(path);
> 
> 	if (fail) {
> 		printf("\n===> BUG REPRODUCED: stale data visible in hole\n");
> 		return 1;
> 	}
> 	printf("\n===> OK: hole reads as zeros on both paths\n");
> 	return 0;
> }
> ------------------------------------------------------
> 
> --
> Nan Lin
> 
> .
> 


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

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

[Nan Lin]

>> Hi Cheng,
>>
>> Thanks for the detailed review. My responses are inline.
>>
>>> The filemap_lock_folio gets folio with FGP_LOCK, I think we should 
>>> use 'FGP_WRITEBEGIN | FGP_ACCESSED' just like ext4_load_tail_bh() 
>>> does, because the folio may not exist in ram when calling do_setattr().
>>
>> ext4_load_tail_bh() needs FGP_CREAT because in Ext4 the block behind 
>> an unwritten/delayed extent holds garbage on disk, so the zeroing must 
>> reach the media. UBIFS does not have this problem -- stale data can 
>> only live in the page cache, never on flash:
>>
>>    writeback:  ubifs_writepage() zeroes the straddling folio on every
>>                invocation and skips folios fully beyond i_size, so
>>                stale tails never reach the media.
>>    read-in:    do_readpage()/read_block() zero the tails of holes and
>>                short data nodes, so a folio loaded from flash is always
>>                tail-clean.
>>
>> So when the EOF folio is absent from the cache, there is nothing to 
>> zero anywhere: FGP_CREAT would merely allocate a fresh (already zero) 
>> folio to zero it again. Returning early on IS_ERR() is the correct and 
>> cheaper equivalent of Ext4's path.
>
>There is one case makes me confused:
>File f, isize = 0
>1. setattr(f, isize = 3)
>    do_setattr -> ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and returns, nothing happens 2. pwrite(f, "123", offset = 6, len = 3)
>    ubifs_write_begin
>     ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and returns, nothing happens
>     folio = __filemap_get_folio(FGP_WRITEBEGIN) // create a new folio with random content
>     if (!folio_test_uptodate(folio))
>       do_readpage // does nothing
>        while (1) {
>          ENOENT = read_block
>        }
>    ubifs_write_end
>     isize = 9
>
>   ubifs_writepage
>    folio content looks like
>   | random | random | 123   |   zero       |
>   0        3        6       9             4096
>

do_readpage() does run here: pos(6) != folio_pos(0), so the
skipped_read branch is not taken. With i_size = 3:

  do_readpage():
    beyond = (3 + 4095) >> 12 = 1
    block(0) < beyond(1) -> no early exit
    read_block(block=0):
      ubifs_tnc_lookup() -> -ENOENT (no data nodes yet)
      -> folio_zero_range(folio, 0, 4096)     /* whole 4K block */
      -> return -ENOENT                       /* file.c:52-59 */
    hole branch: err = 0, folio_set_checked(), folio_mark_uptodate()

So read_block() zeroes the full 4K block *before* returning -ENOENT. The folio VFS copies "123" into is:

  | 0 ... 0 | 123 | 0 ... 0 |
  0         3/6   9        4096

not "| random | random | 123 |". The random bytes never survive:
every !uptodate folio passes through do_readpage(), and read_block()
zeroes every block whose TNC lookup returns -ENOENT. 

>According to the current implementations, could above process happen?
>If it is, I think we need to do two points:
>1. The folio(even not cached) zero range is necessary when setting a new
>bigger size?       -- For 0~3 case
>2. The folio(even not cached) zero range is necessary when writting
>starts after old isize?       -- For 3~6 case
>I think zeroing the whole folio in ubifs_read_folio(ENOENT case) is a simple solution?
>>
>>> Why skipping 'from == 0'? What if a user writes begin from
>>> offset-X(0<X<4096) to a new file(isize = 0)? Ext4 zero the range by 
>>> ext4_block_write_begin->folio_zero_segments(folio, to, block_end, 
>>> block_start, from), but ubifs not handle it.
>>
>> from == 0 means the old EOF is folio-aligned, so the EOF folio has no 
>> bytes beyond EOF and there is no hole in it:
>>
>>    from != 0:  |/////valid/////|stale|   <- zero [from, to)
>>    from == 0:  |/////valid/////|         <- nothing beyond EOF here
>>
>> For the i_size == 0 case: a zero-size file cannot hold a polluted 
>> folio. mmap cannot create one (page_mkwrite returns SIGBUS when 
>> folio_pos >= i_size), and write() followed by ftruncate(0) invalidates 
>> the folio. The [0, X) part of the write folio itself is zeroed by the 
>> existing do_readpage() path ("block >= beyond" -> folio_zero_range()), 
>> which is UBIFS's counterpart of ext4_block_write_begin()'s 
>> folio_zero_segments() -- the work happens at folio read-in because 
>> UBIFS's block size always equals the page size.
>>
>
>This question is similar with the previous one.
>
>>> The ext4 protection check is ext4_write_checks->ext4_block_zero_eof,
>>> I think we can add the zero hole check in ubifs_write_iter().
>>
>> Agreed. This mirrors ext4_write_checks() -> ext4_block_zero_eof() and 
>> runs once per syscall instead of once per write_begin() iteration.
>> Will move it there in v3.
>>
>>> If the program enters into this branch, the ubifs file's isize is not 
>>> zero, ubifs_read_folio has already zerod tail, do we still need this 
>>> folio_zero_range?
>>> Oh, there is one case, UBIFS file has isize without data nodes(eg.
>>> isize = 3, the first block is a data hole), ubifs_read_folio won't 
>>> zero the page tail, we still need folio_zero_range().
>>
>> Right -- and one more case on top of the hole-inode one: mkwrite also 
>> runs on already-cached folios, and earlier mmap stores may have 
>> re-dirtied the tail after ubifs_read_folio() cleaned it (only folios 
>> fully beyond EOF get SIGBUS; the straddling folio stays writable 
>> across its whole length). So the tail must be re-zeroed on every 
>> (re-)fault, and the folio_zero_range() stays.
>
>I;m still confusing about the necessary of this folio_zero_range().
>For a hole inode(isize = 3, no data nodes), if the folio is cached, ubifs_read_folio has zerod the whole folio content(Suspend we have applied the simple solution from the previous question). The subsequent dirty setting will not write the page content after the 3rd byte to a data node.

Yes, I agree: ubifs_read_folio() zeroes the tail at read-in, and
writeback does not persist bytes beyond EOF (the straddling folio's
tail is zeroed and do_writepage() only writes i_size
bytes). My concern is the cached tail of the EOF folio: a previous
shared mapping may have modified it, and a later write fault may
reuse the uptodate folio without calling ubifs_read_folio() again.
The zeroing will clear that stale tail before page_mkwrite() returns.

This is not a general fix for read-only re-mappings, since those do
not invoke page_mkwrite(). In this scenario, I see the hunk as a
defensive cleanup of the cached EOF tail on a later write fault,
rather than a fix for writeback correctness. A userspace reproducer
may race with writeback. I'd lean toward keeping it as a defensive
measure, but I haven't yet demonstrated a correctness issue without
it. What do you think?

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

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

[Zhihao Cheng]

在 2026/10/22 21:15, Nan Lin 写道:
>>> Hi Cheng,
>>>
>>> Thanks for the detailed review. My responses are inline.
>>>
>>>> The filemap_lock_folio gets folio with FGP_LOCK, I think we should
>>>> use 'FGP_WRITEBEGIN | FGP_ACCESSED' just like ext4_load_tail_bh()
>>>> does, because the folio may not exist in ram when calling do_setattr().
>>>
>>> ext4_load_tail_bh() needs FGP_CREAT because in Ext4 the block behind
>>> an unwritten/delayed extent holds garbage on disk, so the zeroing must
>>> reach the media. UBIFS does not have this problem -- stale data can
>>> only live in the page cache, never on flash:
>>>
>>>     writeback:  ubifs_writepage() zeroes the straddling folio on every
>>>                 invocation and skips folios fully beyond i_size, so
>>>                 stale tails never reach the media.
>>>     read-in:    do_readpage()/read_block() zero the tails of holes and
>>>                 short data nodes, so a folio loaded from flash is always
>>>                 tail-clean.
>>>
>>> So when the EOF folio is absent from the cache, there is nothing to
>>> zero anywhere: FGP_CREAT would merely allocate a fresh (already zero)
>>> folio to zero it again. Returning early on IS_ERR() is the correct and
>>> cheaper equivalent of Ext4's path.
>>
>> There is one case makes me confused:
>> File f, isize = 0
>> 1. setattr(f, isize = 3)
>>     do_setattr -> ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and returns, nothing happens 2. pwrite(f, "123", offset = 6, len = 3)
>>     ubifs_write_begin
>>      ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and returns, nothing happens
>>      folio = __filemap_get_folio(FGP_WRITEBEGIN) // create a new folio with random content
>>      if (!folio_test_uptodate(folio))
>>        do_readpage // does nothing
>>         while (1) {
>>           ENOENT = read_block
>>         }
>>     ubifs_write_end
>>      isize = 9
>>
>>    ubifs_writepage
>>     folio content looks like
>>    | random | random | 123   |   zero       |
>>    0        3        6       9             4096
>>
> 
> do_readpage() does run here: pos(6) != folio_pos(0), so the
> skipped_read branch is not taken. With i_size = 3:
> 
>    do_readpage():
>      beyond = (3 + 4095) >> 12 = 1
>      block(0) < beyond(1) -> no early exit
>      read_block(block=0):
>        ubifs_tnc_lookup() -> -ENOENT (no data nodes yet)
Oh, I missed the folio_zero_range in read_block(), sorry for the noisy.
>        -> folio_zero_range(folio, 0, 4096)     /* whole 4K block */
>        -> return -ENOENT                       /* file.c:52-59 */
>      hole branch: err = 0, folio_set_checked(), folio_mark_uptodate()
> 
> So read_block() zeroes the full 4K block *before* returning -ENOENT. The folio VFS copies "123" into is:
> 
>    | 0 ... 0 | 123 | 0 ... 0 |
>    0         3/6   9        4096
> 
> not "| random | random | 123 |". The random bytes never survive:
> every !uptodate folio passes through do_readpage(), and read_block()
> zeroes every block whose TNC lookup returns -ENOENT.
> 
>> According to the current implementations, could above process happen?
>> If it is, I think we need to do two points:
>> 1. The folio(even not cached) zero range is necessary when setting a new
>> bigger size?       -- For 0~3 case
>> 2. The folio(even not cached) zero range is necessary when writting
>> starts after old isize?       -- For 3~6 case
>> I think zeroing the whole folio in ubifs_read_folio(ENOENT case) is a simple solution?
>>>
>>>> Why skipping 'from == 0'? What if a user writes begin from
>>>> offset-X(0<X<4096) to a new file(isize = 0)? Ext4 zero the range by
>>>> ext4_block_write_begin->folio_zero_segments(folio, to, block_end,
>>>> block_start, from), but ubifs not handle it.
>>>
>>> from == 0 means the old EOF is folio-aligned, so the EOF folio has no
>>> bytes beyond EOF and there is no hole in it:
>>>
>>>     from != 0:  |/////valid/////|stale|   <- zero [from, to)
>>>     from == 0:  |/////valid/////|         <- nothing beyond EOF here
>>>
>>> For the i_size == 0 case: a zero-size file cannot hold a polluted
>>> folio. mmap cannot create one (page_mkwrite returns SIGBUS when
>>> folio_pos >= i_size), and write() followed by ftruncate(0) invalidates
>>> the folio. The [0, X) part of the write folio itself is zeroed by the
>>> existing do_readpage() path ("block >= beyond" -> folio_zero_range()),
>>> which is UBIFS's counterpart of ext4_block_write_begin()'s
>>> folio_zero_segments() -- the work happens at folio read-in because
>>> UBIFS's block size always equals the page size.
>>>
>>
>> This question is similar with the previous one.
>>
>>>> The ext4 protection check is ext4_write_checks->ext4_block_zero_eof,
>>>> I think we can add the zero hole check in ubifs_write_iter().
>>>
>>> Agreed. This mirrors ext4_write_checks() -> ext4_block_zero_eof() and
>>> runs once per syscall instead of once per write_begin() iteration.
>>> Will move it there in v3.
>>>
>>>> If the program enters into this branch, the ubifs file's isize is not
>>>> zero, ubifs_read_folio has already zerod tail, do we still need this
>>>> folio_zero_range?
>>>> Oh, there is one case, UBIFS file has isize without data nodes(eg.
>>>> isize = 3, the first block is a data hole), ubifs_read_folio won't
>>>> zero the page tail, we still need folio_zero_range().
>>>
>>> Right -- and one more case on top of the hole-inode one: mkwrite also
>>> runs on already-cached folios, and earlier mmap stores may have
>>> re-dirtied the tail after ubifs_read_folio() cleaned it (only folios
>>> fully beyond EOF get SIGBUS; the straddling folio stays writable
>>> across its whole length). So the tail must be re-zeroed on every
>>> (re-)fault, and the folio_zero_range() stays.
>>
>> I;m still confusing about the necessary of this folio_zero_range().
>> For a hole inode(isize = 3, no data nodes), if the folio is cached, ubifs_read_folio has zerod the whole folio content(Suspend we have applied the simple solution from the previous question). The subsequent dirty setting will not write the page content after the 3rd byte to a data node.
> 
> Yes, I agree: ubifs_read_folio() zeroes the tail at read-in, and
> writeback does not persist bytes beyond EOF (the straddling folio's
> tail is zeroed and do_writepage() only writes i_size
> bytes). My concern is the cached tail of the EOF folio: a previous
> shared mapping may have modified it, and a later write fault may
> reuse the uptodate folio without calling ubifs_read_folio() again.
> The zeroing will clear that stale tail before page_mkwrite() returns.
> 
Agreed with the analysis of the multi writing processes. Anyway, user 
must be aware that he is modifying a file beyonds EOF by mmap(). The 
user should be responsible for the memory content beyonds EOF.
> This is not a general fix for read-only re-mappings, since those do
> not invoke page_mkwrite(). In this scenario, I see the hunk as a
> defensive cleanup of the cached EOF tail on a later write fault,
> rather than a fix for writeback correctness. A userspace reproducer
> may race with writeback. I'd lean toward keeping it as a defensive
> measure, but I haven't yet demonstrated a correctness issue without
> it. What do you think?
> 

Well, this is not a practical issue and will not have any impact on the 
user, and other filesystems(eg. ext4) behave like UBIFS for this case, 
so I prefer to keep the original implementation.


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

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

[Nan Lin]

>>>> Hi Cheng,
>>>>
>>>> Thanks for the detailed review. My responses are inline.
>>>>
>>>>> The filemap_lock_folio gets folio with FGP_LOCK, I think we should 
>>>>> use 'FGP_WRITEBEGIN | FGP_ACCESSED' just like ext4_load_tail_bh() 
>>>>> does, because the folio may not exist in ram when calling do_setattr().
>>>>
>>>> ext4_load_tail_bh() needs FGP_CREAT because in Ext4 the block behind 
>>>> an unwritten/delayed extent holds garbage on disk, so the zeroing 
>>>> must reach the media. UBIFS does not have this problem -- stale data 
>>>> can only live in the page cache, never on flash:
>>>>
>>>>     writeback:  ubifs_writepage() zeroes the straddling folio on every
>>>>                 invocation and skips folios fully beyond i_size, so
>>>>                 stale tails never reach the media.
>>>>     read-in:    do_readpage()/read_block() zero the tails of holes and
>>>>                 short data nodes, so a folio loaded from flash is always
>>>>                 tail-clean.
>>>>
>>>> So when the EOF folio is absent from the cache, there is nothing to 
>>>> zero anywhere: FGP_CREAT would merely allocate a fresh (already 
>>>> zero) folio to zero it again. Returning early on IS_ERR() is the 
>>>> correct and cheaper equivalent of Ext4's path.
>>>
>>> There is one case makes me confused:
>>> File f, isize = 0
>>> 1. setattr(f, isize = 3)
>>>     do_setattr -> ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and returns, nothing happens 2. pwrite(f, "123", offset = 6, len = 3)
>>>     ubifs_write_begin
>>>      ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and returns, nothing happens
>>>      folio = __filemap_get_folio(FGP_WRITEBEGIN) // create a new folio with random content
>>>      if (!folio_test_uptodate(folio))
>>>        do_readpage // does nothing
>>>         while (1) {
>>>           ENOENT = read_block
>>>         }
>>>     ubifs_write_end
>>>      isize = 9
>>>
>>>    ubifs_writepage
>>>     folio content looks like
>>>    | random | random | 123   |   zero       |
>>>    0        3        6       9             4096
>>>
>>
>> do_readpage() does run here: pos(6) != folio_pos(0), so the 
>> skipped_read branch is not taken. With i_size = 3:
>>
>>    do_readpage():
>>      beyond = (3 + 4095) >> 12 = 1
>>      block(0) < beyond(1) -> no early exit
>>      read_block(block=0):
>>        ubifs_tnc_lookup() -> -ENOENT (no data nodes yet)
>Oh, I missed the folio_zero_range in read_block(), sorry for the noisy.
>>        -> folio_zero_range(folio, 0, 4096)     /* whole 4K block */
>>        -> return -ENOENT                       /* file.c:52-59 */
>>      hole branch: err = 0, folio_set_checked(), folio_mark_uptodate()
>>
>> So read_block() zeroes the full 4K block *before* returning -ENOENT. The folio VFS copies "123" into is:
>>
>>    | 0 ... 0 | 123 | 0 ... 0 |
>>    0         3/6   9        4096
>>
>> not "| random | random | 123 |". The random bytes never survive:
>> every !uptodate folio passes through do_readpage(), and read_block() 
>> zeroes every block whose TNC lookup returns -ENOENT.
>>
>>> According to the current implementations, could above process happen?
>>> If it is, I think we need to do two points:
>>> 1. The folio(even not cached) zero range is necessary when setting a new
>>> bigger size?       -- For 0~3 case
>>> 2. The folio(even not cached) zero range is necessary when writting
>>> starts after old isize?       -- For 3~6 case
>>> I think zeroing the whole folio in ubifs_read_folio(ENOENT case) is a simple solution?
>>>>
>>>>> Why skipping 'from == 0'? What if a user writes begin from
>>>>> offset-X(0<X<4096) to a new file(isize = 0)? Ext4 zero the range by 
>>>>> ext4_block_write_begin->folio_zero_segments(folio, to, block_end, 
>>>>> block_start, from), but ubifs not handle it.
>>>>
>>>> from == 0 means the old EOF is folio-aligned, so the EOF folio has 
>>>> no bytes beyond EOF and there is no hole in it:
>>>>
>>>>     from != 0:  |/////valid/////|stale|   <- zero [from, to)
>>>>     from == 0:  |/////valid/////|         <- nothing beyond EOF here
>>>>
>>>> For the i_size == 0 case: a zero-size file cannot hold a polluted 
>>>> folio. mmap cannot create one (page_mkwrite returns SIGBUS when 
>>>> folio_pos >= i_size), and write() followed by ftruncate(0) 
>>>> invalidates the folio. The [0, X) part of the write folio itself is 
>>>> zeroed by the existing do_readpage() path ("block >= beyond" -> 
>>>> folio_zero_range()), which is UBIFS's counterpart of 
>>>> ext4_block_write_begin()'s
>>>> folio_zero_segments() -- the work happens at folio read-in because 
>>>> UBIFS's block size always equals the page size.
>>>>
>>>
>>> This question is similar with the previous one.
>>>
>>>>> The ext4 protection check is 
>>>>> ext4_write_checks->ext4_block_zero_eof,
>>>>> I think we can add the zero hole check in ubifs_write_iter().
>>>>
>>>> Agreed. This mirrors ext4_write_checks() -> ext4_block_zero_eof() 
>>>> and runs once per syscall instead of once per write_begin() iteration.
>>>> Will move it there in v3.
>>>>
>>>>> If the program enters into this branch, the ubifs file's isize is 
>>>>> not zero, ubifs_read_folio has already zerod tail, do we still need 
>>>>> this folio_zero_range?
>>>>> Oh, there is one case, UBIFS file has isize without data nodes(eg.
>>>>> isize = 3, the first block is a data hole), ubifs_read_folio won't 
>>>>> zero the page tail, we still need folio_zero_range().
>>>>
>>>> Right -- and one more case on top of the hole-inode one: mkwrite 
>>>> also runs on already-cached folios, and earlier mmap stores may have 
>>>> re-dirtied the tail after ubifs_read_folio() cleaned it (only folios 
>>>> fully beyond EOF get SIGBUS; the straddling folio stays writable 
>>>> across its whole length). So the tail must be re-zeroed on every 
>>>> (re-)fault, and the folio_zero_range() stays.
>>>
>>> I;m still confusing about the necessary of this folio_zero_range().
>>> For a hole inode(isize = 3, no data nodes), if the folio is cached, ubifs_read_folio has zerod the whole folio content(Suspend we have applied the simple solution from the previous question). The subsequent dirty setting will not write the page content after the 3rd byte to a data node.
>>
>> Yes, I agree: ubifs_read_folio() zeroes the tail at read-in, and 
>> writeback does not persist bytes beyond EOF (the straddling folio's 
>> tail is zeroed and do_writepage() only writes i_size bytes). My 
>> concern is the cached tail of the EOF folio: a previous shared mapping 
>> may have modified it, and a later write fault may reuse the uptodate 
>> folio without calling ubifs_read_folio() again.
>> The zeroing will clear that stale tail before page_mkwrite() returns.
>>
>Agreed with the analysis of the multi writing processes. Anyway, user must be aware that he is modifying a file beyonds EOF by mmap(). The user should be responsible for the memory content beyonds EOF.
>> This is not a general fix for read-only re-mappings, since those do 
>> not invoke page_mkwrite(). In this scenario, I see the hunk as a 
>> defensive cleanup of the cached EOF tail on a later write fault, 
>> rather than a fix for writeback correctness. A userspace reproducer 
>> may race with writeback. I'd lean toward keeping it as a defensive 
>> measure, but I haven't yet demonstrated a correctness issue without 
>> it. What do you think?
>>
>
>Well, this is not a practical issue and will not have any impact on the user, and other filesystems(eg. ext4) behave like UBIFS for this case, so I prefer to keep the original implementation.

Understood. I verified your point against the code:
ext4_page_mkwrite() -> ext4_block_page_mkwrite() ->
ext4_block_write_begin(handle, folio, 0, len) with len =
i_size - folio_pos, and nothing zeroes [i_size, folio_size) on the
write fault path; block_page_mkwrite() behaves the same. I agree
the mmap-past-EOF semantics put the responsibility on the user.

I'll drop the mkwrite hunk in v3 and fix the commit message
accordingly (drop the inaccurate "mirrors block_page_mkwrite()"
claim, refer to the ext4 size-growth zeroing instead), keeping the
write_iter() and do_setattr() zeroing. Will post after testing.

Best regards,
Nan Lin


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

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

[Zhihao Cheng]

在 2026/10/23 9:23, Nan Lin 写道:
>>>>> Hi Cheng,
>>>>>
>>>>> Thanks for the detailed review. My responses are inline.
>>>>>
>>>>>> The filemap_lock_folio gets folio with FGP_LOCK, I think we should
>>>>>> use 'FGP_WRITEBEGIN | FGP_ACCESSED' just like ext4_load_tail_bh()
>>>>>> does, because the folio may not exist in ram when calling do_setattr().
>>>>>
>>>>> ext4_load_tail_bh() needs FGP_CREAT because in Ext4 the block behind
>>>>> an unwritten/delayed extent holds garbage on disk, so the zeroing
>>>>> must reach the media. UBIFS does not have this problem -- stale data
>>>>> can only live in the page cache, never on flash:
>>>>>
>>>>>      writeback:  ubifs_writepage() zeroes the straddling folio on every
>>>>>                  invocation and skips folios fully beyond i_size, so
>>>>>                  stale tails never reach the media.
>>>>>      read-in:    do_readpage()/read_block() zero the tails of holes and
>>>>>                  short data nodes, so a folio loaded from flash is always
>>>>>                  tail-clean.
>>>>>
>>>>> So when the EOF folio is absent from the cache, there is nothing to
>>>>> zero anywhere: FGP_CREAT would merely allocate a fresh (already
>>>>> zero) folio to zero it again. Returning early on IS_ERR() is the
>>>>> correct and cheaper equivalent of Ext4's path.
>>>>
>>>> There is one case makes me confused:
>>>> File f, isize = 0
>>>> 1. setattr(f, isize = 3)
>>>>      do_setattr -> ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and returns, nothing happens 2. pwrite(f, "123", offset = 6, len = 3)
>>>>      ubifs_write_begin
>>>>       ubifs_zero_eof_hole -> filemap_lock_folio // gets no folio and returns, nothing happens
>>>>       folio = __filemap_get_folio(FGP_WRITEBEGIN) // create a new folio with random content
>>>>       if (!folio_test_uptodate(folio))
>>>>         do_readpage // does nothing
>>>>          while (1) {
>>>>            ENOENT = read_block
>>>>          }
>>>>      ubifs_write_end
>>>>       isize = 9
>>>>
>>>>     ubifs_writepage
>>>>      folio content looks like
>>>>     | random | random | 123   |   zero       |
>>>>     0        3        6       9             4096
>>>>
>>>
>>> do_readpage() does run here: pos(6) != folio_pos(0), so the
>>> skipped_read branch is not taken. With i_size = 3:
>>>
>>>     do_readpage():
>>>       beyond = (3 + 4095) >> 12 = 1
>>>       block(0) < beyond(1) -> no early exit
>>>       read_block(block=0):
>>>         ubifs_tnc_lookup() -> -ENOENT (no data nodes yet)
>> Oh, I missed the folio_zero_range in read_block(), sorry for the noisy.
>>>         -> folio_zero_range(folio, 0, 4096)     /* whole 4K block */
>>>         -> return -ENOENT                       /* file.c:52-59 */
>>>       hole branch: err = 0, folio_set_checked(), folio_mark_uptodate()
>>>
>>> So read_block() zeroes the full 4K block *before* returning -ENOENT. The folio VFS copies "123" into is:
>>>
>>>     | 0 ... 0 | 123 | 0 ... 0 |
>>>     0         3/6   9        4096
>>>
>>> not "| random | random | 123 |". The random bytes never survive:
>>> every !uptodate folio passes through do_readpage(), and read_block()
>>> zeroes every block whose TNC lookup returns -ENOENT.
>>>
>>>> According to the current implementations, could above process happen?
>>>> If it is, I think we need to do two points:
>>>> 1. The folio(even not cached) zero range is necessary when setting a new
>>>> bigger size?       -- For 0~3 case
>>>> 2. The folio(even not cached) zero range is necessary when writting
>>>> starts after old isize?       -- For 3~6 case
>>>> I think zeroing the whole folio in ubifs_read_folio(ENOENT case) is a simple solution?
>>>>>
>>>>>> Why skipping 'from == 0'? What if a user writes begin from
>>>>>> offset-X(0<X<4096) to a new file(isize = 0)? Ext4 zero the range by
>>>>>> ext4_block_write_begin->folio_zero_segments(folio, to, block_end,
>>>>>> block_start, from), but ubifs not handle it.
>>>>>
>>>>> from == 0 means the old EOF is folio-aligned, so the EOF folio has
>>>>> no bytes beyond EOF and there is no hole in it:
>>>>>
>>>>>      from != 0:  |/////valid/////|stale|   <- zero [from, to)
>>>>>      from == 0:  |/////valid/////|         <- nothing beyond EOF here
>>>>>
>>>>> For the i_size == 0 case: a zero-size file cannot hold a polluted
>>>>> folio. mmap cannot create one (page_mkwrite returns SIGBUS when
>>>>> folio_pos >= i_size), and write() followed by ftruncate(0)
>>>>> invalidates the folio. The [0, X) part of the write folio itself is
>>>>> zeroed by the existing do_readpage() path ("block >= beyond" ->
>>>>> folio_zero_range()), which is UBIFS's counterpart of
>>>>> ext4_block_write_begin()'s
>>>>> folio_zero_segments() -- the work happens at folio read-in because
>>>>> UBIFS's block size always equals the page size.
>>>>>
>>>>
>>>> This question is similar with the previous one.
>>>>
>>>>>> The ext4 protection check is
>>>>>> ext4_write_checks->ext4_block_zero_eof,
>>>>>> I think we can add the zero hole check in ubifs_write_iter().
>>>>>
>>>>> Agreed. This mirrors ext4_write_checks() -> ext4_block_zero_eof()
>>>>> and runs once per syscall instead of once per write_begin() iteration.
>>>>> Will move it there in v3.
>>>>>
>>>>>> If the program enters into this branch, the ubifs file's isize is
>>>>>> not zero, ubifs_read_folio has already zerod tail, do we still need
>>>>>> this folio_zero_range?
>>>>>> Oh, there is one case, UBIFS file has isize without data nodes(eg.
>>>>>> isize = 3, the first block is a data hole), ubifs_read_folio won't
>>>>>> zero the page tail, we still need folio_zero_range().
>>>>>
>>>>> Right -- and one more case on top of the hole-inode one: mkwrite
>>>>> also runs on already-cached folios, and earlier mmap stores may have
>>>>> re-dirtied the tail after ubifs_read_folio() cleaned it (only folios
>>>>> fully beyond EOF get SIGBUS; the straddling folio stays writable
>>>>> across its whole length). So the tail must be re-zeroed on every
>>>>> (re-)fault, and the folio_zero_range() stays.
>>>>
>>>> I;m still confusing about the necessary of this folio_zero_range().
>>>> For a hole inode(isize = 3, no data nodes), if the folio is cached, ubifs_read_folio has zerod the whole folio content(Suspend we have applied the simple solution from the previous question). The subsequent dirty setting will not write the page content after the 3rd byte to a data node.
>>>
>>> Yes, I agree: ubifs_read_folio() zeroes the tail at read-in, and
>>> writeback does not persist bytes beyond EOF (the straddling folio's
>>> tail is zeroed and do_writepage() only writes i_size bytes). My
>>> concern is the cached tail of the EOF folio: a previous shared mapping
>>> may have modified it, and a later write fault may reuse the uptodate
>>> folio without calling ubifs_read_folio() again.
>>> The zeroing will clear that stale tail before page_mkwrite() returns.
>>>
>> Agreed with the analysis of the multi writing processes. Anyway, user must be aware that he is modifying a file beyonds EOF by mmap(). The user should be responsible for the memory content beyonds EOF.
>>> This is not a general fix for read-only re-mappings, since those do
>>> not invoke page_mkwrite(). In this scenario, I see the hunk as a
>>> defensive cleanup of the cached EOF tail on a later write fault,
>>> rather than a fix for writeback correctness. A userspace reproducer
>>> may race with writeback. I'd lean toward keeping it as a defensive
>>> measure, but I haven't yet demonstrated a correctness issue without
>>> it. What do you think?
>>>
>>
>> Well, this is not a practical issue and will not have any impact on the user, and other filesystems(eg. ext4) behave like UBIFS for this case, so I prefer to keep the original implementation.
> 
> Understood. I verified your point against the code:
> ext4_page_mkwrite() -> ext4_block_page_mkwrite() ->
> ext4_block_write_begin(handle, folio, 0, len) with len =
> i_size - folio_pos, and nothing zeroes [i_size, folio_size) on the
> write fault path; block_page_mkwrite() behaves the same. I agree
> the mmap-past-EOF semantics put the responsibility on the user.
> 
> I'll drop the mkwrite hunk in v3 and fix the commit message
> accordingly (drop the inaccurate "mirrors block_page_mkwrite()"
> claim, refer to the ext4 size-growth zeroing instead), keeping the
> write_iter() and do_setattr() zeroing. Will post after testing.
> 

Sounds fine to me.
> Best regards,
> Nan Lin
> 
> 
> .
> 


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

[PATCH v3] 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 cached old-EOF folio in both size-extending
paths: ubifs_write_iter() for writes starting beyond EOF, and
do_setattr() before truncate_setsize() grows the size. This mirrors
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(); the reverse order
deadlocks.

UBIFS does not enable large folios, so the EOF folio is always a
single page today; nevertheless, use folio-relative arithmetic
(offset_in_folio(), folio_contains(), folio_size()) throughout so the
code is correct if that ever changes.

Signed-off-by: Nan Lin <lin.nan@h3c.com>

Changes since v2:
 - Move the extending-write check from ubifs_write_begin() to
   ubifs_write_iter(), mirroring ext4_write_checks() ->
   ext4_block_zero_eof(): the hole zeroing runs once per write()
   syscall instead of once per ->write_begin() iteration (suggested
   by Zhihao Cheng).
 - Drop the ubifs_vm_page_mkwrite() zeroing: like ext4
   (ext4_block_write_begin() with len = i_size - folio_pos),
   ->page_mkwrite() does not zero the tail beyond EOF, and mmap
   writes past EOF are the user's responsibility (suggested by
   Zhihao Cheng).

Changes since v1:
 - Both zeroing sites used PAGE_SIZE-relative arithmetic
   (offset_in_page()-style) while folio_zero_range() expects
   folio-relative offsets. UBIFS does not currently enable large
   folios, so PAGE_SIZE == folio_size() and the two coincide in
   practice, but switch to offset_in_folio()/folio_size() anyway so
   the code is correct if UBIFS ever opts into large folios
   (suggested by Sashiko AI review).
---
 fs/ubifs/file.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index e73c28b12f97..e94fbcae6d3d 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);
+	size_t 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 = offset_in_folio(folio, old_size);
+	if (!from)
+		goto out;
+	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
+	     offset_in_folio(folio, new_end) : folio_size(folio);
+	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)
 {
@@ -1220,6 +1267,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);
 	}
 
@@ -1433,7 +1484,17 @@ static int update_mctime(struct inode *inode)
 
 static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	int err = update_mctime(file_inode(iocb->ki_filp));
+	struct inode *inode = file_inode(iocb->ki_filp);
+	int err;
+
+	/*
+	 * The write may extend beyond EOF: zero the stale tail of the
+	 * old-EOF folio first.
+	 */
+	if (iocb->ki_pos > i_size_read(inode))
+		ubifs_zero_eof_hole(inode, iocb->ki_pos);
+
+	err = update_mctime(inode);
 	if (err)
 		return err;
 
-- 
2.33.0


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

Re: [PATCH v3] 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:
- [Critical] ubifs: unlocked ubifs_zero_eof_hole() in ubifs_write_iter() corrupts data
--

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

ubifs: zero stale data beyond EOF before file size grows

This commit adds ubifs_zero_eof_hole() and calls it from do_setattr() and
ubifs_write_iter() to zero stale post-EOF data in the cached old-EOF folio
before extending a file's size.

> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index e73c28b12f97f..e94fbcae6d3d5 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);
> +	size_t 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 = offset_in_folio(folio, old_size);
> +	if (!from)
> +		goto out;
> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
> +	     offset_in_folio(folio, new_end) : folio_size(folio);
> +	if (to > from)
> +		folio_zero_range(folio, from, to - from);
> +
> +out:
> +	folio_unlock(folio);
> +	folio_put(folio);
> +}

[ ... ]

> @@ -1433,7 +1484,17 @@ static int update_mctime(struct inode *inode)
>  
>  static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  {
> -	int err = update_mctime(file_inode(iocb->ki_filp));
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +	int err;
> +
> +	/*
> +	 * The write may extend beyond EOF: zero the stale tail of the
> +	 * old-EOF folio first.
> +	 */
> +	if (iocb->ki_pos > i_size_read(inode))
> +		ubifs_zero_eof_hole(inode, iocb->ki_pos);
> +
> +	err = update_mctime(inode);
>  	if (err)
>  		return err;

[Severity: Critical]
Unlike do_setattr() (called with inode_lock(inode) held via notify_change())
and ext4_buffered_write_iter() -> ext4_write_checks() ->
ext4_block_zero_eof(), ubifs_write_iter() calls ubifs_zero_eof_hole() before
generic_file_write_iter() acquires inode_lock(inode) and runs
generic_write_checks(iocb, from):

mm/filemap.c:generic_file_write_iter() {
	inode_lock(inode);
	ret = generic_write_checks(iocb, from);
	if (ret > 0)
		ret = __generic_file_write_iter(iocb, from);
	inode_unlock(inode);
}

Can calling ubifs_zero_eof_hole() without holding inode_lock(inode) and
before generic_write_checks() cause data corruption and other regressions?

1. If ubifs_zero_eof_hole() reads an initial old_size in folio 0 (for
example, old_size = 100) and calls filemap_lock_folio() on index 0, a
concurrent writer or extending ftruncate() holding inode_lock(inode) can
extend inode->i_size into a later folio (for example, old_size becomes 5100
in folio 1, while new_end is 10000 in folio 2) before folio 0 is locked.
At that point, all of folio 0 is valid committed file data.

When ubifs_zero_eof_hole() acquires the lock on folio 0 and re-reads
old_size = i_size_read(inode), new_end <= old_size is still false, and there
is no check that folio_contains(folio, old_size >> PAGE_SHIFT). Computing
from = offset_in_folio(folio, old_size) masks the new old_size into folio 0
and calls folio_zero_range(folio, 1004, 3092) on folio 0, zeroing committed
in-bounds file data while leaving the actual old-EOF folio (folio 1)
unzeroed.

A concurrent shrinking truncate that moves i_size into an earlier folio
before truncate_pagecache() evicts the locked folio will similarly zero the
wrong folio. And if a concurrent extending ftruncate() updates i_size in
truncate_setsize() right after ubifs_zero_eof_hole() re-reads old_size,
userspace mmap writes to the newly in-bounds region can be overwritten by
folio_zero_range().

2. Because inode_lock(inode) is not held and folio_lock is dropped before
update_mctime() (which can sleep in ubifs_budget_space()) and before
generic_file_write_iter() locks the inode, a concurrent truncate or write can
change i_size or an mmap write can re-dirty the post-EOF tail in the window
between ubifs_zero_eof_hole() and __generic_file_write_iter(), leaving stale
data beyond EOF unzeroed.

3. Because generic_write_checks() has not run yet:
- For O_APPEND or RWF_APPEND writes where ki_pos was positioned past EOF,
  generic_write_checks_count() later resets iocb->ki_pos to i_size_read(inode)
  so no hole is created, but ubifs_zero_eof_hole() has already modified the
  page cache.
- For zero-length writes (iov_iter_count(from) == 0) with ki_pos > i_size,
  generic_write_checks() returns 0 without extending the file, after
  ubifs_zero_eof_hole() has already zeroed the folio tail.
- For RWF_NOWAIT writes (which generic_write_checks_count() rejects with
  -EINVAL since FOP_BUFFER_WASYNC is not set) or writes failing
  generic_write_check_limits() (-EFBIG), ubifs_zero_eof_hole() can block on
  filemap_lock_folio() and mutate the page cache before the write is
  rejected.

Should ubifs_write_iter() acquire inode_lock(inode) and run
generic_write_checks() before calling update_mctime(),
ubifs_zero_eof_hole(), and __generic_file_write_iter(), or otherwise ensure
that ubifs_zero_eof_hole() runs under inode_lock(inode) after write checks?

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

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

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

[Zhihao Cheng]

在 2026/10/23 15:35, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] ubifs: unlocked ubifs_zero_eof_hole() in ubifs_write_iter() corrupts data
> --
> 
> commit f10b956c6f33e7d0c8aba1ed8f979c3d9e1197bf
> Author: Nan Lin <lin.nan@h3c.com>
> 
> ubifs: zero stale data beyond EOF before file size grows
> 
> This commit adds ubifs_zero_eof_hole() and calls it from do_setattr() and
> ubifs_write_iter() to zero stale post-EOF data in the cached old-EOF folio
> before extending a file's size.
> 
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index e73c28b12f97f..e94fbcae6d3d5 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);
>> +	size_t 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 = offset_in_folio(folio, old_size);
>> +	if (!from)
>> +		goto out;
>> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
>> +	     offset_in_folio(folio, new_end) : folio_size(folio);
>> +	if (to > from)
>> +		folio_zero_range(folio, from, to - from);
>> +
>> +out:
>> +	folio_unlock(folio);
>> +	folio_put(folio);
>> +}
> 
> [ ... ]
> 
>> @@ -1433,7 +1484,17 @@ static int update_mctime(struct inode *inode)
>>   
>>   static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>   {
>> -	int err = update_mctime(file_inode(iocb->ki_filp));
>> +	struct inode *inode = file_inode(iocb->ki_filp);
>> +	int err;
>> +
>> +	/*
>> +	 * The write may extend beyond EOF: zero the stale tail of the
>> +	 * old-EOF folio first.
>> +	 */
>> +	if (iocb->ki_pos > i_size_read(inode))
>> +		ubifs_zero_eof_hole(inode, iocb->ki_pos);
>> +
>> +	err = update_mctime(inode);
>>   	if (err)
>>   		return err;
> 
Hi, Nan Lin
I think the sashiko report is an effective improvement. The inode_lock 
is needed, and zero eof check should be executed after 
generic_write_checks().

> [Severity: Critical]
> Unlike do_setattr() (called with inode_lock(inode) held via notify_change())
> and ext4_buffered_write_iter() -> ext4_write_checks() ->
> ext4_block_zero_eof(), ubifs_write_iter() calls ubifs_zero_eof_hole() before
> generic_file_write_iter() acquires inode_lock(inode) and runs
> generic_write_checks(iocb, from):
> 
> mm/filemap.c:generic_file_write_iter() {
> 	inode_lock(inode);
> 	ret = generic_write_checks(iocb, from);
> 	if (ret > 0)
> 		ret = __generic_file_write_iter(iocb, from);
> 	inode_unlock(inode);
> }
> 
> Can calling ubifs_zero_eof_hole() without holding inode_lock(inode) and
> before generic_write_checks() cause data corruption and other regressions?
> 
> 1. If ubifs_zero_eof_hole() reads an initial old_size in folio 0 (for
> example, old_size = 100) and calls filemap_lock_folio() on index 0, a
> concurrent writer or extending ftruncate() holding inode_lock(inode) can
> extend inode->i_size into a later folio (for example, old_size becomes 5100
> in folio 1, while new_end is 10000 in folio 2) before folio 0 is locked.
> At that point, all of folio 0 is valid committed file data.
> 
> When ubifs_zero_eof_hole() acquires the lock on folio 0 and re-reads
> old_size = i_size_read(inode), new_end <= old_size is still false, and there
> is no check that folio_contains(folio, old_size >> PAGE_SHIFT). Computing
> from = offset_in_folio(folio, old_size) masks the new old_size into folio 0
> and calls folio_zero_range(folio, 1004, 3092) on folio 0, zeroing committed
> in-bounds file data while leaving the actual old-EOF folio (folio 1)
> unzeroed.
> 
> A concurrent shrinking truncate that moves i_size into an earlier folio
> before truncate_pagecache() evicts the locked folio will similarly zero the
> wrong folio. And if a concurrent extending ftruncate() updates i_size in
> truncate_setsize() right after ubifs_zero_eof_hole() re-reads old_size,
> userspace mmap writes to the newly in-bounds region can be overwritten by
> folio_zero_range().
> 
> 2. Because inode_lock(inode) is not held and folio_lock is dropped before
> update_mctime() (which can sleep in ubifs_budget_space()) and before
> generic_file_write_iter() locks the inode, a concurrent truncate or write can
> change i_size or an mmap write can re-dirty the post-EOF tail in the window
> between ubifs_zero_eof_hole() and __generic_file_write_iter(), leaving stale
> data beyond EOF unzeroed.
> 
> 3. Because generic_write_checks() has not run yet:
> - For O_APPEND or RWF_APPEND writes where ki_pos was positioned past EOF,
>    generic_write_checks_count() later resets iocb->ki_pos to i_size_read(inode)
>    so no hole is created, but ubifs_zero_eof_hole() has already modified the
>    page cache.
> - For zero-length writes (iov_iter_count(from) == 0) with ki_pos > i_size,
>    generic_write_checks() returns 0 without extending the file, after
>    ubifs_zero_eof_hole() has already zeroed the folio tail.
> - For RWF_NOWAIT writes (which generic_write_checks_count() rejects with
>    -EINVAL since FOP_BUFFER_WASYNC is not set) or writes failing
>    generic_write_check_limits() (-EFBIG), ubifs_zero_eof_hole() can block on
>    filemap_lock_folio() and mutate the page cache before the write is
>    rejected.
> 
> Should ubifs_write_iter() acquire inode_lock(inode) and run
> generic_write_checks() before calling update_mctime(),
> ubifs_zero_eof_hole(), and __generic_file_write_iter(), or otherwise ensure
> that ubifs_zero_eof_hole() runs under inode_lock(inode) after write checks?
> 


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

[PATCH v4] 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 cached old-EOF folio in both size-extending
paths: ubifs_write_iter() for writes starting beyond EOF, and
do_setattr() before truncate_setsize() grows the size. This mirrors
what ext4 does on size extension;

Restructure ubifs_write_iter() to match ext4_buffered_write_iter():
acquire inode_lock, perform write checks, call kiocb_modified() for
setuid/setgid clearing, update mtime/ctime via update_mctime()
(UBIFS sets S_NOCMTIME so VFS time updates short-circuit), zero the
EOF hole, then execute the write with generic_perform_write(). This
ensures proper serialization and security handling.

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

UBIFS does not enable large folios, so the EOF folio is always a
single page today; nevertheless, use folio-relative arithmetic
(offset_in_folio(), folio_contains(), folio_size()) throughout so the
code is correct if that ever changes.

Signed-off-by: Nan Lin <lin.nan@h3c.com>

Changes since v3:
 - Add inode_lock and move zero-eof after generic_write_checks(),
   matching ext4_buffered_write_iter structure
   (suggested by Sashiko AI review & Zhihao Cheng).
 - Add kiocb_modified() for setuid/setgid clearing.
 - Keep explicit update_mctime() as UBIFS sets S_NOCMTIME.

Changes since v2:
 - Move the extending-write check from ubifs_write_begin() to
   ubifs_write_iter(), mirroring ext4_write_checks() ->
   ext4_block_zero_eof(): the hole zeroing runs once per write()
   syscall instead of once per ->write_begin() iteration (suggested
   by Zhihao Cheng).
 - Drop the ubifs_vm_page_mkwrite() zeroing: like ext4
   (ext4_block_write_begin() with len = i_size - folio_pos),
   ->page_mkwrite() does not zero the tail beyond EOF, and mmap
   writes past EOF are the user's responsibility (suggested by
   Zhihao Cheng).

Changes since v1:
 - Both zeroing sites used PAGE_SIZE-relative arithmetic
   (offset_in_page()-style) while folio_zero_range() expects
   folio-relative offsets. UBIFS does not currently enable large
   folios, so PAGE_SIZE == folio_size() and the two coincide in
   practice, but switch to offset_in_folio()/folio_size() anyway so
   the code is correct if UBIFS ever opts into large folios
   (suggested by Sashiko AI review).
---
 fs/ubifs/file.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 82 insertions(+), 4 deletions(-)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index e73c28b12f97..6f2c83b6f433 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -202,6 +202,45 @@ 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 with inode_lock(inode) held and 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);
+	size_t 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;
+
+	from = offset_in_folio(folio, old_size);
+	if (!from)
+		goto out;
+	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
+	     offset_in_folio(folio, new_end) : folio_size(folio);
+	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)
 {
@@ -1220,6 +1259,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);
 	}
 
@@ -1433,11 +1476,46 @@ static int update_mctime(struct inode *inode)
 
 static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	int err = update_mctime(file_inode(iocb->ki_filp));
-	if (err)
-		return err;
+	struct inode *inode = file_inode(iocb->ki_filp);
+	ssize_t ret;
+
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		return -EOPNOTSUPP;
+
+	inode_lock(inode);
+
+	ret = generic_write_checks(iocb, from);
+	if (ret <= 0)
+		goto out;
+
+	ret = kiocb_modified(iocb);
+	if (ret)
+		goto out;
 
-	return generic_file_write_iter(iocb, from);
+	/*
+	 * UBIFS sets S_NOCMTIME on all inodes, so VFS's file_update_time_flags()
+	 * short-circuits and never calls ubifs_update_time() for mtime/ctime.
+	 * We must call update_mctime() explicitly here.
+	 */
+	ret = update_mctime(inode);
+	if (ret)
+		goto out;
+
+	/*
+	 * If the write position is beyond EOF, zero the stale tail of the
+	 * old-EOF folio first.  generic_write_checks() has already resolved
+	 * O_APPEND/RWF_APPEND and rejected invalid writes, so iocb->ki_pos
+	 * is the final write position.
+	 */
+	if (iocb->ki_pos > i_size_read(inode))
+		ubifs_zero_eof_hole(inode, iocb->ki_pos);
+
+	ret = generic_perform_write(iocb, from);
+out:
+	inode_unlock(inode);
+	if (ret > 0)
+		ret = generic_write_sync(iocb, ret);
+	return ret;
 }
 
 static bool ubifs_dirty_folio(struct address_space *mapping,
-- 
2.33.0


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

Re: [PATCH v4] 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: stale mmap data exposed on short copy in ubifs_write_iter()
--

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

ubifs: zero stale data beyond EOF before file size grows

This commit adds ubifs_zero_eof_hole() to zero stale mmap data beyond EOF in
the old-EOF folio during do_setattr() and ubifs_write_iter(), and restructures
ubifs_write_iter() to serialize writes under inode_lock().

> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index e73c28b12f97f..6f2c83b6f4333 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -202,6 +202,45 @@ 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 with inode_lock(inode) held and 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);
> +	size_t 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;
> +
> +	from = offset_in_folio(folio, old_size);
> +	if (!from)
> +		goto out;
> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
> +	     offset_in_folio(folio, new_end) : folio_size(folio);
> +	if (to > from)
> +		folio_zero_range(folio, from, to - from);
> +
> +out:
> +	folio_unlock(folio);
> +	folio_put(folio);
> +}

[ ... ]

> @@ -1433,11 +1476,46 @@ static int update_mctime(struct inode *inode)
>  
>  static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  {
> -	int err = update_mctime(file_inode(iocb->ki_filp));
> -	if (err)
> -		return err;
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +	ssize_t ret;
> +
> +	if (iocb->ki_flags & IOCB_NOWAIT)
> +		return -EOPNOTSUPP;
> +
> +	inode_lock(inode);
> +
> +	ret = generic_write_checks(iocb, from);
> +	if (ret <= 0)
> +		goto out;
> +
> +	ret = kiocb_modified(iocb);
> +	if (ret)
> +		goto out;
>  
> -	return generic_file_write_iter(iocb, from);
> +	/*
> +	 * UBIFS sets S_NOCMTIME on all inodes, so VFS's file_update_time_flags()
> +	 * short-circuits and never calls ubifs_update_time() for mtime/ctime.
> +	 * We must call update_mctime() explicitly here.
> +	 */
> +	ret = update_mctime(inode);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * If the write position is beyond EOF, zero the stale tail of the
> +	 * old-EOF folio first.  generic_write_checks() has already resolved
> +	 * O_APPEND/RWF_APPEND and rejected invalid writes, so iocb->ki_pos
> +	 * is the final write position.
> +	 */
> +	if (iocb->ki_pos > i_size_read(inode))
> +		ubifs_zero_eof_hole(inode, iocb->ki_pos);
> +
> +	ret = generic_perform_write(iocb, from);

[Severity: High]
Can stale mmap data still be exposed when generic_perform_write() encounters a
short or zero-byte copy in copy_folio_from_iter_atomic()?

Suppose a file has a non-page-aligned size old_size and a shared mmap writes
beyond EOF, leaving the old-EOF folio uptodate with stale data in
[old_size, folio_size(folio)).

When a subsequent write() or pwrite() starts at pos >= old_size within that
same folio, ubifs_write_iter() skips ubifs_zero_eof_hole() if pos == old_size,
or only zeroes [old_size, pos) if pos > old_size, leaving [pos, folio_size(folio))
containing the stale mmap data under the assumption that the write will
overwrite [pos, pos + len).

However, if copy_folio_from_iter_atomic() copies fewer than len bytes (for
example, copied == 0 due to an unmapped user buffer returning -EFAULT, or an
unfaulted user page when a signal is pending), ubifs_write_end() only takes
the short-copy bailout when !folio_test_uptodate(folio), and otherwise extends
i_size to end_pos = pos + len rather than pos + copied:

fs/ubifs/file.c:ubifs_write_end() {
	loff_t end_pos = pos + len;
	int appending = !!(end_pos > inode->i_size);
	...
	if (unlikely(copied < len && !folio_test_uptodate(folio))) {
		...
		goto out;
	}
	...
	if (appending) {
		i_size_write(inode, end_pos);
		ui->ui_size = end_pos;
		...
	}
}

Because the old-EOF folio is already uptodate, wouldn't ubifs_write_end()
advance inode->i_size and ui->ui_size to pos + len even when copied < len,
exposing the unzeroed stale mmap data in [pos + copied, pos + len)?

By comparison, ext4_block_zero_eof() zeroes the entire tail of the old-EOF
folio from old_size to folio_size(folio), and ext4_write_end() only advances
i_size to pos + copied. Should ubifs_zero_eof_hole() also zero to the end of
the folio (or be called whenever the write extends past i_size), or should
ubifs_write_end() avoid extending i_size past pos + copied?

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

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

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

[Nan Lin]

>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index e73c28b12f97f..6f2c83b6f4333 100644
>> --- a/fs/ubifs/file.c
>> +++ b/fs/ubifs/file.c
>> @@ -202,6 +202,45 @@ 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 with inode_lock(inode) held and 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);
>> +	size_t 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;
>> +
>> +	from = offset_in_folio(folio, old_size);
>> +	if (!from)
>> +		goto out;
>> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
>> +	     offset_in_folio(folio, new_end) : folio_size(folio);
>> +	if (to > from)
>> +		folio_zero_range(folio, from, to - from);
>> +
>> +out:
>> +	folio_unlock(folio);
>> +	folio_put(folio);
>> +}
>
>[ ... ]
>
>> @@ -1433,11 +1476,46 @@ static int update_mctime(struct inode *inode)
>>  
>>  static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>  {
>> -	int err = update_mctime(file_inode(iocb->ki_filp));
>> -	if (err)
>> -		return err;
>> +	struct inode *inode = file_inode(iocb->ki_filp);
>> +	ssize_t ret;
>> +
>> +	if (iocb->ki_flags & IOCB_NOWAIT)
>> +		return -EOPNOTSUPP;
>> +
>> +	inode_lock(inode);
>> +
>> +	ret = generic_write_checks(iocb, from);
>> +	if (ret <= 0)
>> +		goto out;
>> +
>> +	ret = kiocb_modified(iocb);
>> +	if (ret)
>> +		goto out;
>>  
>> -	return generic_file_write_iter(iocb, from);
>> +	/*
>> +	 * UBIFS sets S_NOCMTIME on all inodes, so VFS's file_update_time_flags()
>> +	 * short-circuits and never calls ubifs_update_time() for mtime/ctime.
>> +	 * We must call update_mctime() explicitly here.
>> +	 */
>> +	ret = update_mctime(inode);
>> +	if (ret)
>> +		goto out;
>> +
>> +	/*
>> +	 * If the write position is beyond EOF, zero the stale tail of the
>> +	 * old-EOF folio first.  generic_write_checks() has already resolved
>> +	 * O_APPEND/RWF_APPEND and rejected invalid writes, so iocb->ki_pos
>> +	 * is the final write position.
>> +	 */
>> +	if (iocb->ki_pos > i_size_read(inode))
>> +		ubifs_zero_eof_hole(inode, iocb->ki_pos);
>> +
>> +	ret = generic_perform_write(iocb, from);
>
>[Severity: High]
>Can stale mmap data still be exposed when generic_perform_write() encounters a
>short or zero-byte copy in copy_folio_from_iter_atomic()?
>
>Suppose a file has a non-page-aligned size old_size and a shared mmap writes
>beyond EOF, leaving the old-EOF folio uptodate with stale data in
>[old_size, folio_size(folio)).
>
>When a subsequent write() or pwrite() starts at pos >= old_size within that
>same folio, ubifs_write_iter() skips ubifs_zero_eof_hole() if pos == old_size,
>or only zeroes [old_size, pos) if pos > old_size, leaving [pos, folio_size(folio))
>containing the stale mmap data under the assumption that the write will
>overwrite [pos, pos + len).
>
>However, if copy_folio_from_iter_atomic() copies fewer than len bytes (for
>example, copied == 0 due to an unmapped user buffer returning -EFAULT, or an
>unfaulted user page when a signal is pending), ubifs_write_end() only takes
>the short-copy bailout when !folio_test_uptodate(folio), and otherwise extends
>i_size to end_pos = pos + len rather than pos + copied:
>
>fs/ubifs/file.c:ubifs_write_end() {
>	loff_t end_pos = pos + len;
>	int appending = !!(end_pos > inode->i_size);
>	...
>	if (unlikely(copied < len && !folio_test_uptodate(folio))) {
>		...
>		goto out;
>	}
>	...
>	if (appending) {
>		i_size_write(inode, end_pos);
>		ui->ui_size = end_pos;
>		...
>	}
>}
>
>Because the old-EOF folio is already uptodate, wouldn't ubifs_write_end()
>advance inode->i_size and ui->ui_size to pos + len even when copied < len,
>exposing the unzeroed stale mmap data in [pos + copied, pos + len)?
>
>By comparison, ext4_block_zero_eof() zeroes the entire tail of the old-EOF
>folio from old_size to folio_size(folio), and ext4_write_end() only advances
>i_size to pos + copied. Should ubifs_zero_eof_hole() also zero to the end of
>the folio (or be called whenever the write extends past i_size), or should
>ubifs_write_end() avoid extending i_size past pos + copied?

Valid concern. I think the short-copy issue is best handled separately
since it involves write_end() budget semantics and affects all buffered
writes, not just extending ones. Note that the i_size advance to pos + len on
short copies is pre-existing UBIFS behavior, not introduced by this patch. 
Worth a dedicated analysis/patch but outside the scope of this fix.

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

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

[Zhihao Cheng]

在 2026/10/27 14:12, Nan Lin 写道:
>>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> 
[...]
> 
>> Because the old-EOF folio is already uptodate, wouldn't ubifs_write_end()
> 
>> advance inode->i_size and ui->ui_size to pos + len even when copied < len,
> 
>> exposing the unzeroed stale mmap data in [pos + copied, pos + len)?
> 
>>
> 
>> By comparison, ext4_block_zero_eof() zeroes the entire tail of the old-EOF
> 
>> folio from old_size to folio_size(folio), and ext4_write_end() only advances
> 
>> i_size to pos + copied. Should ubifs_zero_eof_hole() also zero to the end of
> 
>> the folio (or be called whenever the write extends past i_size), or should
> 
>> ubifs_write_end() avoid extending i_size past pos + copied?
> 
> 
> 
> Valid concern. I think the short-copy issue is best handled separately
> 
> since it involves write_end() budget semantics and affects all buffered
> 
> writes, not just extending ones. Note that the i_size advance to pos + len on
> 
> short copies is pre-existing UBIFS behavior, not introduced by this patch.
> 
> Worth a dedicated analysis/patch but outside the scope of this fix.
> 

Agreed
> 
> .
> 


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

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

[Zhihao Cheng]

在 2026/10/26 20:50, 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 cached old-EOF folio in both size-extending
> paths: ubifs_write_iter() for writes starting beyond EOF, and
> do_setattr() before truncate_setsize() grows the size. This mirrors
> what ext4 does on size extension;
> 
> Restructure ubifs_write_iter() to match ext4_buffered_write_iter():
> acquire inode_lock, perform write checks, call kiocb_modified() for
> setuid/setgid clearing, update mtime/ctime via update_mctime()
> (UBIFS sets S_NOCMTIME so VFS time updates short-circuit), zero the
> EOF hole, then execute the write with generic_perform_write(). This
> ensures proper serialization and security handling.
> 
> All zeroing takes the old-EOF folio lock without ui->ui_mutex, keeping
> the folio_lock -> ui_mutex order of ->writepage(); the reverse order
> deadlocks.
> 
> UBIFS does not enable large folios, so the EOF folio is always a
> single page today; nevertheless, use folio-relative arithmetic
> (offset_in_folio(), folio_contains(), folio_size()) throughout so the
> code is correct if that ever changes.
> 
> Signed-off-by: Nan Lin <lin.nan@h3c.com>
> 
> Changes since v3:
>   - Add inode_lock and move zero-eof after generic_write_checks(),
>     matching ext4_buffered_write_iter structure
>     (suggested by Sashiko AI review & Zhihao Cheng).
>   - Add kiocb_modified() for setuid/setgid clearing.
>   - Keep explicit update_mctime() as UBIFS sets S_NOCMTIME.
> 
> Changes since v2:
>   - Move the extending-write check from ubifs_write_begin() to
>     ubifs_write_iter(), mirroring ext4_write_checks() ->
>     ext4_block_zero_eof(): the hole zeroing runs once per write()
>     syscall instead of once per ->write_begin() iteration (suggested
>     by Zhihao Cheng).
>   - Drop the ubifs_vm_page_mkwrite() zeroing: like ext4
>     (ext4_block_write_begin() with len = i_size - folio_pos),
>     ->page_mkwrite() does not zero the tail beyond EOF, and mmap
>     writes past EOF are the user's responsibility (suggested by
>     Zhihao Cheng).
> 
> Changes since v1:
>   - Both zeroing sites used PAGE_SIZE-relative arithmetic
>     (offset_in_page()-style) while folio_zero_range() expects
>     folio-relative offsets. UBIFS does not currently enable large
>     folios, so PAGE_SIZE == folio_size() and the two coincide in
>     practice, but switch to offset_in_folio()/folio_size() anyway so
>     the code is correct if UBIFS ever opts into large folios
>     (suggested by Sashiko AI review).
> ---
>   fs/ubifs/file.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 82 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index e73c28b12f97..6f2c83b6f433 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -202,6 +202,45 @@ 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 with inode_lock(inode) held and 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);
> +	size_t 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;
> +
> +	from = offset_in_folio(folio, old_size);
> +	if (!from)
> +		goto out;
> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
> +	     offset_in_folio(folio, new_end) : folio_size(folio);
> +	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)
>   {
> @@ -1220,6 +1259,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);
>   	}
>   
> @@ -1433,11 +1476,46 @@ static int update_mctime(struct inode *inode)
>   
>   static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   {
> -	int err = update_mctime(file_inode(iocb->ki_filp));
> -	if (err)
> -		return err;
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +	ssize_t ret;
> +
> +	if (iocb->ki_flags & IOCB_NOWAIT)
> +		return -EOPNOTSUPP;
> +
> +	inode_lock(inode);
> +
> +	ret = generic_write_checks(iocb, from);
> +	if (ret <= 0)
> +		goto out;
> +
> +	ret = kiocb_modified(iocb);
> +	if (ret)
> +		goto out;
Hi,
I think file_remove_privs() is enouth, because ubifs inode has 
S_NOCMTIME flag and the iocb->ki_flags without 'IOCB_NOWAIT'.
>   
> -	return generic_file_write_iter(iocb, from);
> +	/*
> +	 * UBIFS sets S_NOCMTIME on all inodes, so VFS's file_update_time_flags()
> +	 * short-circuits and never calls ubifs_update_time() for mtime/ctime.
> +	 * We must call update_mctime() explicitly here.
> +	 */
> +	ret = update_mctime(inode);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * If the write position is beyond EOF, zero the stale tail of the
> +	 * old-EOF folio first.  generic_write_checks() has already resolved
> +	 * O_APPEND/RWF_APPEND and rejected invalid writes, so iocb->ki_pos
> +	 * is the final write position.
> +	 */
> +	if (iocb->ki_pos > i_size_read(inode))
> +		ubifs_zero_eof_hole(inode, iocb->ki_pos);
> +
> +	ret = generic_perform_write(iocb, from);
> +out:
> +	inode_unlock(inode);
> +	if (ret > 0)
> +		ret = generic_write_sync(iocb, ret);
> +	return ret;
>   }
>   
>   static bool ubifs_dirty_folio(struct address_space *mapping,
> 


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

[PATCH v4] 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 cached old-EOF folio in both size-extending
>> paths: ubifs_write_iter() for writes starting beyond EOF, and
>> do_setattr() before truncate_setsize() grows the size. This mirrors
>> what ext4 does on size extension;
>> 
>> Restructure ubifs_write_iter() to match ext4_buffered_write_iter():
>> acquire inode_lock, perform write checks, call kiocb_modified() for
>> setuid/setgid clearing, update mtime/ctime via update_mctime()
>> (UBIFS sets S_NOCMTIME so VFS time updates short-circuit), zero the
>> EOF hole, then execute the write with generic_perform_write(). This
>> ensures proper serialization and security handling.
>> 
>> All zeroing takes the old-EOF folio lock without ui->ui_mutex, keeping
>> the folio_lock -> ui_mutex order of ->writepage(); the reverse order
>> deadlocks.
>> 
>> UBIFS does not enable large folios, so the EOF folio is always a
>> single page today; nevertheless, use folio-relative arithmetic
>> (offset_in_folio(), folio_contains(), folio_size()) throughout so the
>> code is correct if that ever changes.
>> 
>> Signed-off-by: Nan Lin <lin.nan@h3c.com>
>> 
>> Changes since v3:
>>   - Add inode_lock and move zero-eof after generic_write_checks(),
>>     matching ext4_buffered_write_iter structure
>>     (suggested by Sashiko AI review & Zhihao Cheng).
>>   - Add kiocb_modified() for setuid/setgid clearing.
>>   - Keep explicit update_mctime() as UBIFS sets S_NOCMTIME.
>> 
>> Changes since v2:
>>   - Move the extending-write check from ubifs_write_begin() to
>>     ubifs_write_iter(), mirroring ext4_write_checks() ->
>>     ext4_block_zero_eof(): the hole zeroing runs once per write()
>>     syscall instead of once per ->write_begin() iteration (suggested
>>     by Zhihao Cheng).
>>   - Drop the ubifs_vm_page_mkwrite() zeroing: like ext4
>>     (ext4_block_write_begin() with len = i_size - folio_pos),
>>     ->page_mkwrite() does not zero the tail beyond EOF, and mmap
>>     writes past EOF are the user's responsibility (suggested by
>>     Zhihao Cheng).
>> 
>> Changes since v1:
>>   - Both zeroing sites used PAGE_SIZE-relative arithmetic
>>     (offset_in_page()-style) while folio_zero_range() expects
>>     folio-relative offsets. UBIFS does not currently enable large
>>     folios, so PAGE_SIZE == folio_size() and the two coincide in
>>     practice, but switch to offset_in_folio()/folio_size() anyway so
>>     the code is correct if UBIFS ever opts into large folios
>>     (suggested by Sashiko AI review).
>> ---
>>   fs/ubifs/file.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 82 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index e73c28b12f97..6f2c83b6f433 100644
>> --- a/fs/ubifs/file.c
>> +++ b/fs/ubifs/file.c
>> @@ -202,6 +202,45 @@ 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 with inode_lock(inode) held and 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);
>> +	size_t 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;
>> +
>> +	from = offset_in_folio(folio, old_size);
>> +	if (!from)
>> +		goto out;
>> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
>> +	     offset_in_folio(folio, new_end) : folio_size(folio);
>> +	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)
>>   {
>> @@ -1220,6 +1259,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);
>>   	}
>>   
>> @@ -1433,11 +1476,46 @@ static int update_mctime(struct inode *inode)
>>   
>>   static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>   {
>> -	int err = update_mctime(file_inode(iocb->ki_filp));
>> -	if (err)
>> -		return err;
>> +	struct inode *inode = file_inode(iocb->ki_filp);
>> +	ssize_t ret;
>> +
>> +	if (iocb->ki_flags & IOCB_NOWAIT)
>> +		return -EOPNOTSUPP;
>> +
>> +	inode_lock(inode);
>> +
>> +	ret = generic_write_checks(iocb, from);
>> +	if (ret <= 0)
>> +		goto out;
>> +
>> +	ret = kiocb_modified(iocb);
>> +	if (ret)
>> +		goto out;
>Hi,
>I think file_remove_privs() is enouth, because ubifs inode has 
>S_NOCMTIME flag and the iocb->ki_flags without 'IOCB_NOWAIT'.

Ack, This makes the code clearer.
I'll make this change in the next revision.

>>   
>> -	return generic_file_write_iter(iocb, from);
>> +	/*
>> +	 * UBIFS sets S_NOCMTIME on all inodes, so VFS's file_update_time_flags()
>> +	 * short-circuits and never calls ubifs_update_time() for mtime/ctime.
>> +	 * We must call update_mctime() explicitly here.
>> +	 */
>> +	ret = update_mctime(inode);
>> +	if (ret)
>> +		goto out;
>> +
>> +	/*
>> +	 * If the write position is beyond EOF, zero the stale tail of the
>> +	 * old-EOF folio first.  generic_write_checks() has already resolved
>> +	 * O_APPEND/RWF_APPEND and rejected invalid writes, so iocb->ki_pos
>> +	 * is the final write position.
>> +	 */
>> +	if (iocb->ki_pos > i_size_read(inode))
>> +		ubifs_zero_eof_hole(inode, iocb->ki_pos);
>> +
>> +	ret = generic_perform_write(iocb, from);
>> +out:
>> +	inode_unlock(inode);
>> +	if (ret > 0)
>> +		ret = generic_write_sync(iocb, ret);
>> +	return ret;
>>   }
>>   
>>   static bool ubifs_dirty_folio(struct address_space *mapping,
>> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5] 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 cached old-EOF folio in both size-extending
paths: ubifs_write_iter() for writes starting beyond EOF, and
do_setattr() before truncate_setsize() grows the size. This mirrors
what ext4 does on size extension;

Restructure ubifs_write_iter() to match ext4_buffered_write_iter():
acquire inode_lock, perform write checks, call file_remove_privs() for
setuid/setgid clearing (UBIFS sets S_NOCMTIME so file_modified() is a
no-op), update mtime/ctime via update_mctime(), zero the EOF hole,
then execute the write with generic_perform_write(). This ensures
proper serialization and security handling.

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

UBIFS does not enable large folios, so the EOF folio is always a
single page today; nevertheless, use folio-relative arithmetic
(offset_in_folio(), folio_contains(), folio_size()) throughout so the
code is correct if that ever changes.

Signed-off-by: Nan Lin <lin.nan@h3c.com>

Changes since v4:
 - Use file_remove_privs() instead of kiocb_modified(), as UBIFS sets
   S_NOCMTIME making file_modified() a no-op (suggested by
   Zhihao Cheng).

Changes since v3:
 - Add inode_lock and move zero-eof after generic_write_checks(),
   matching ext4_buffered_write_iter structure
   (suggested by Sashiko AI review & Zhihao Cheng).
 - Add kiocb_modified() for setuid/setgid clearing.
 - Keep explicit update_mctime() as UBIFS sets S_NOCMTIME.

Changes since v2:
 - Move the extending-write check from ubifs_write_begin() to
   ubifs_write_iter(), mirroring ext4_write_checks() ->
   ext4_block_zero_eof(): the hole zeroing runs once per write()
   syscall instead of once per ->write_begin() iteration (suggested
   by Zhihao Cheng).
 - Drop the ubifs_vm_page_mkwrite() zeroing: like ext4
   (ext4_block_write_begin() with len = i_size - folio_pos),
   ->page_mkwrite() does not zero the tail beyond EOF, and mmap
   writes past EOF are the user's responsibility (suggested by
   Zhihao Cheng).

Changes since v1:
 - Both zeroing sites used PAGE_SIZE-relative arithmetic
   (offset_in_page()-style) while folio_zero_range() expects
   folio-relative offsets. UBIFS does not currently enable large
   folios, so PAGE_SIZE == folio_size() and the two coincide in
   practice, but switch to offset_in_folio()/folio_size() anyway so
   the code is correct if UBIFS ever opts into large folios
   (suggested by Sashiko AI review).

Note: The short-copy issue in ubifs_write_end() where i_size may
advance to pos + len on partial copies is pre-existing UBIFS behavior,
not introduced by this patch. It affects all buffered writes and
involves write_end() budget/ui_mutex handling; worth a dedicated
analysis/patch but outside the scope of this fix.
---
 fs/ubifs/file.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 4 deletions(-)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index e73c28b12f97..534e93793744 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -202,6 +202,44 @@ 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.
+ *
+ * Context: inode_lock held, ui_mutex not held.  The old-EOF folio_lock
+ * must nest outside ui_mutex to match ->writepage()/->page_mkwrite().
+ */
+static void ubifs_zero_eof_hole(struct inode *inode, loff_t new_end)
+{
+	loff_t old_size = i_size_read(inode);
+	size_t 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;
+
+	from = offset_in_folio(folio, old_size);
+	if (!from)
+		goto out;
+	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
+	     offset_in_folio(folio, new_end) : folio_size(folio);
+	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)
 {
@@ -1220,6 +1258,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);
+
+		/* Zero before truncate_setsize() changes i_size. */
+		ubifs_zero_eof_hole(inode, new_size);
+
 		truncate_setsize(inode, new_size);
 	}
 
@@ -1433,11 +1475,41 @@ static int update_mctime(struct inode *inode)
 
 static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	int err = update_mctime(file_inode(iocb->ki_filp));
-	if (err)
-		return err;
+	struct inode *inode = file_inode(iocb->ki_filp);
+	ssize_t ret;
+
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		return -EOPNOTSUPP;
 
-	return generic_file_write_iter(iocb, from);
+	inode_lock(inode);
+
+	ret = generic_write_checks(iocb, from);
+	if (ret <= 0)
+		goto out;
+
+	/* Clear setuid/setgid; skip file_modified() due to S_NOCMTIME. */
+	ret = file_remove_privs(iocb->ki_filp);
+	if (ret)
+		goto out;
+
+	/* Update mtime/ctime explicitly (VFS skips it for S_NOCMTIME). */
+	ret = update_mctime(inode);
+	if (ret)
+		goto out;
+
+	/*
+	 * Zero stale data beyond old EOF if extending.  At this point
+	 * ki_pos is final (O_APPEND/RWF_APPEND already resolved).
+	 */
+	if (iocb->ki_pos > i_size_read(inode))
+		ubifs_zero_eof_hole(inode, iocb->ki_pos);
+
+	ret = generic_perform_write(iocb, from);
+out:
+	inode_unlock(inode);
+	if (ret > 0)
+		ret = generic_write_sync(iocb, ret);
+	return ret;
 }
 
 static bool ubifs_dirty_folio(struct address_space *mapping,
-- 
2.33.0


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

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

[Zhihao Cheng]

在 2026/10/27 16:05, 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 cached old-EOF folio in both size-extending
> paths: ubifs_write_iter() for writes starting beyond EOF, and
> do_setattr() before truncate_setsize() grows the size. This mirrors
> what ext4 does on size extension;
> 
> Restructure ubifs_write_iter() to match ext4_buffered_write_iter():
> acquire inode_lock, perform write checks, call file_remove_privs() for
> setuid/setgid clearing (UBIFS sets S_NOCMTIME so file_modified() is a
> no-op), update mtime/ctime via update_mctime(), zero the EOF hole,
> then execute the write with generic_perform_write(). This ensures
> proper serialization and security handling.
> 
> All zeroing takes the old-EOF folio lock without ui->ui_mutex, keeping
> the folio_lock -> ui_mutex order of ->writepage(); the reverse order
> deadlocks.
> 
> UBIFS does not enable large folios, so the EOF folio is always a
> single page today; nevertheless, use folio-relative arithmetic
> (offset_in_folio(), folio_contains(), folio_size()) throughout so the
> code is correct if that ever changes.
> 
> Signed-off-by: Nan Lin <lin.nan@h3c.com>
> 
> Changes since v4:
>   - Use file_remove_privs() instead of kiocb_modified(), as UBIFS sets
>     S_NOCMTIME making file_modified() a no-op (suggested by
>     Zhihao Cheng).
> 
> Changes since v3:
>   - Add inode_lock and move zero-eof after generic_write_checks(),
>     matching ext4_buffered_write_iter structure
>     (suggested by Sashiko AI review & Zhihao Cheng).
>   - Add kiocb_modified() for setuid/setgid clearing.
>   - Keep explicit update_mctime() as UBIFS sets S_NOCMTIME.
> 
> Changes since v2:
>   - Move the extending-write check from ubifs_write_begin() to
>     ubifs_write_iter(), mirroring ext4_write_checks() ->
>     ext4_block_zero_eof(): the hole zeroing runs once per write()
>     syscall instead of once per ->write_begin() iteration (suggested
>     by Zhihao Cheng).
>   - Drop the ubifs_vm_page_mkwrite() zeroing: like ext4
>     (ext4_block_write_begin() with len = i_size - folio_pos),
>     ->page_mkwrite() does not zero the tail beyond EOF, and mmap
>     writes past EOF are the user's responsibility (suggested by
>     Zhihao Cheng).
> 
> Changes since v1:
>   - Both zeroing sites used PAGE_SIZE-relative arithmetic
>     (offset_in_page()-style) while folio_zero_range() expects
>     folio-relative offsets. UBIFS does not currently enable large
>     folios, so PAGE_SIZE == folio_size() and the two coincide in
>     practice, but switch to offset_in_folio()/folio_size() anyway so
>     the code is correct if UBIFS ever opts into large folios
>     (suggested by Sashiko AI review).
> 
> Note: The short-copy issue in ubifs_write_end() where i_size may
> advance to pos + len on partial copies is pre-existing UBIFS behavior,
> not introduced by this patch. It affects all buffered writes and
> involves write_end() budget/ui_mutex handling; worth a dedicated
> analysis/patch but outside the scope of this fix.
> ---
>   fs/ubifs/file.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 76 insertions(+), 4 deletions(-)
> 

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index e73c28b12f97..534e93793744 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -202,6 +202,44 @@ 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.
> + *
> + * Context: inode_lock held, ui_mutex not held.  The old-EOF folio_lock
> + * must nest outside ui_mutex to match ->writepage()/->page_mkwrite().
> + */
> +static void ubifs_zero_eof_hole(struct inode *inode, loff_t new_end)
> +{
> +	loff_t old_size = i_size_read(inode);
> +	size_t 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;
> +
> +	from = offset_in_folio(folio, old_size);
> +	if (!from)
> +		goto out;
> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
> +	     offset_in_folio(folio, new_end) : folio_size(folio);
> +	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)
>   {
> @@ -1220,6 +1258,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);
> +
> +		/* Zero before truncate_setsize() changes i_size. */
> +		ubifs_zero_eof_hole(inode, new_size);
> +
>   		truncate_setsize(inode, new_size);
>   	}
>   
> @@ -1433,11 +1475,41 @@ static int update_mctime(struct inode *inode)
>   
>   static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   {
> -	int err = update_mctime(file_inode(iocb->ki_filp));
> -	if (err)
> -		return err;
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +	ssize_t ret;
> +
> +	if (iocb->ki_flags & IOCB_NOWAIT)
> +		return -EOPNOTSUPP;
>   
> -	return generic_file_write_iter(iocb, from);
> +	inode_lock(inode);
> +
> +	ret = generic_write_checks(iocb, from);
> +	if (ret <= 0)
> +		goto out;
> +
> +	/* Clear setuid/setgid; skip file_modified() due to S_NOCMTIME. */
> +	ret = file_remove_privs(iocb->ki_filp);
> +	if (ret)
> +		goto out;
> +
> +	/* Update mtime/ctime explicitly (VFS skips it for S_NOCMTIME). */
> +	ret = update_mctime(inode);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * Zero stale data beyond old EOF if extending.  At this point
> +	 * ki_pos is final (O_APPEND/RWF_APPEND already resolved).
> +	 */
> +	if (iocb->ki_pos > i_size_read(inode))
> +		ubifs_zero_eof_hole(inode, iocb->ki_pos);
> +
> +	ret = generic_perform_write(iocb, from);
> +out:
> +	inode_unlock(inode);
> +	if (ret > 0)
> +		ret = generic_write_sync(iocb, ret);
> +	return ret;
>   }
>   
>   static bool ubifs_dirty_folio(struct address_space *mapping,
> 


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