Re: [PATCH] ext4: skip eof zeroing when the zero range covers the eof block

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] ext4: stale data in ext4_alloc_file_blocks() on FALLOC_FL_WRITE_ZEROES
--

commit 5e26fed6c5f3f56bd5013064a243a3a558e3a73d
Author: Baokun Li <libaokun@linux.alibaba.com>

ext4: skip eof zeroing when the zero range covers the eof block

This commit skips calling ext4_block_zero_eof() in ext4_alloc_file_blocks()
when EXT4_GET_BLOCKS_CONVERT_UNWRITTEN is set and the allocation range covers
the EOF block, preventing the dropped EOF page from being read back into the
page cache with stale data during FALLOC_FL_ZERO_RANGE.

> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 7193429a05e1f..8f7ded6e08299 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4639,8 +4639,15 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>  	credits = ext4_chunk_trans_blocks(inode, len_lblk);
>  	depth = ext_depth(inode);
>  
> -	/* Zero to the end of the block containing i_size */
> -	if (new_size > old_size) {
> +	/*
> +	 * Zero to the end of the block containing i_size.
> +	 * Loading the eof block back before it is converted to unwritten
> +	 * may leave the page cache inconsistent with the disk, and zeroing
> +	 * it is redundant, so skip it.
> +	 */
> +	if (new_size > old_size &&
> +	    (!(flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) ||
> +	     !in_range(old_size >> blkbits, map.m_lblk, map.m_len))) {
>  		ret = ext4_block_zero_eof(inode, old_size, LLONG_MAX);
>  		if (ret)
>  			return ret;

[Severity: Critical]
Does this condition also need to check !alloc_zero to cover the
FALLOC_FL_WRITE_ZEROES path?

In ext4_zero_range(), after ext4_truncate_page_cache_block_range() drops the
page cache, FALLOC_FL_WRITE_ZEROES calls ext4_alloc_file_blocks() with
EXT4_GET_BLOCKS_CREATE_ZERO rather than EXT4_GET_BLOCKS_CONVERT_UNWRITTEN:

	if (align_end > align_start) {
		if (mode & FALLOC_FL_WRITE_ZEROES)
			flags = EXT4_GET_BLOCKS_CREATE_ZERO | EXT4_EX_NOCACHE;
		else
			flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN |
				  EXT4_EX_NOCACHE);
		ret = ext4_alloc_file_blocks(file, align_start,
					     align_end - align_start, new_size,
					     flags);

Earlier in ext4_alloc_file_blocks(), EXT4_GET_BLOCKS_ZERO is cleared from
flags and converted into alloc_zero = true:

	if (flags & EXT4_GET_BLOCKS_ZERO) {
		flags &= ~EXT4_GET_BLOCKS_ZERO;
		flags |= EXT4_GET_BLOCKS_UNWRIT_EXT;
		alloc_zero = true;
	}

When FALLOC_FL_WRITE_ZEROES extends i_size across an unaligned written EOF
block covered by [align_start, align_end),
!(flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) is true. As a result,
ext4_block_zero_eof() is still called and reads the dropped EOF block back
from disk into the page cache with stale data below old_size, marks the
buffer or folio dirty, and then the loop zeroes the block on disk via
ext4_issue_zeroout() and leaves the extent mapped as written.

Wouldn't this leave the dirty page cache folio with stale data that is
returned on buffered reads and written back over the zeroed disk block on
writeback or fsync?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022065342.3569693-1-libaokun@linux.alibaba.com?part=1