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

[sashiko-bot at 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 at 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