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

[sashiko-bot@kernel.org]

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/