Re: [PATCH v3] ext4: do not advance i_size on a buffered write that copied nothing

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 10/30/2026 12:05 PM, Hengyu Liang wrote:
> Commit 665575cff098 ("filemap: move prefaulting out of hot write path")
> made generic_perform_write() call ->write_begin() and ->write_end()
> before it finds out that the source buffer cannot be read.  In that
> case ->write_end() is called with copied == 0.
> 
> However, ext4_write_end(), ext4_journalled_write_end() and
> ext4_da_do_write_end() set i_size to pos + copied without checking
> copied.  As of now, a buffered write() past EOF that fails with EFAULT
> grows the file to the write offset, and the new size can reach the
> disk.
> 
> The issue can be reproduced on an ext4 mount with:
> 
>     char *p = mmap(NULL, 4096, PROT_NONE,
>                    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>     int fd = open("/mnt/ext4/file", O_CREAT | O_TRUNC | O_RDWR, 0600);
>     pwrite(fd, p, 4096, 1024 * 1024);   /* fails with EFAULT */
>     fstat(fd, &st);
> 
> Before that commit, the result is st_size = 0.
> After it, the result is st_size = 1048576.
> 
> This patch will leave i_size alone when nothing was copied, as
> ext4_write_inline_data_end() already does.  Unlike ext4_write_end(),
> the delalloc path does not trim after a short write, so the folio and
> the delayed blocks that ->write_begin() set up beyond i_size are left
> behind, and the blocks stay reserved until the inode is evicted.  When
> the write ends beyond i_size, drop them as well.
> 
> Fixes: 665575cff098 ("filemap: move prefaulting out of hot write path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>

Thank you for the update! This looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>


Please note that I went through the three scenarios reported by sashiko.
I think the first one is a false positive, and I had already sent a
patch to fix the third one [1].

The second scenario, however, is indeed a real issue. But I think it is
a pre-existing problem and needs a separate fix, because there is
another way to end up with a half-converted inline inode passing
through. That is, a truncate to 0 can catch up in the middle between
ext4_da_convert_inline_data_to_extent() starting the conversion and
ext4_do_writepages() completing it. I'm not sure whether this outcome is
expected, though, since apart from the subsequent ext4_write_begin()
path, I don't see any other damage.

Perhaps we should, in ext4_write_begin(), after the
EXT4_STATE_MAY_INLINE_DATA snippet, also check whether there is still
inline data, and if so call something like ext4_convert_inline_data() to
let it finish the conversion before copying data.

[1] https://lore.kernel.org/linux-ext4/20261030021338.1768292-3-yi.zhang@huaweicloud.com/

Thanks,
Yi.

> ---
> Changes in v3:
> - Also trim after a short copy: drop the '!copied' check and compare
>   pos + len with i_size after the update, as ext4_write_end() does
>   (Zhang Yi).
> - Reword the last paragraph of the commit message.
> - v2: https://lore.kernel.org/r/20261023172532.3271348-1-hengyul@cs.unc.edu
> 
> Tested in KVM with KASAN: the reproducer above, failed writes around
> EOF, and short copies on delalloc, nodelalloc and 1k block filesystems.
> e2fsck -fn reports no errors.
> 
>  fs/ext4/inode.c | 39 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6ede3644a738..9c20876629bc 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1578,15 +1578,18 @@ static int ext4_write_end(const struct kiocb *iocb,
>  	 * it's important to update i_size while still holding folio lock:
>  	 * page writeout could otherwise come in and zero beyond i_size.
>  	 *
> +	 * If nothing was copied (e.g. the source buffer was unreadable), do
> +	 * not move i_size up to pos.
> +	 *
>  	 * If FS_IOC_ENABLE_VERITY is running on this inode, then Merkle tree
>  	 * blocks are being written past EOF, so skip the i_size update.
>  	 */
> -	if (!verity)
> +	if (copied && !verity)
>  		i_size_changed = ext4_update_inode_size(inode, pos + copied);
>  	folio_unlock(folio);
>  	folio_put(folio);
>  
> -	if (old_size < pos && !verity)
> +	if (copied && old_size < pos && !verity)
>  		pagecache_isize_extended(inode, old_size, pos);
>  
>  	/*
> @@ -1697,13 +1700,13 @@ static int ext4_journalled_write_end(const struct kiocb *iocb,
>  		if (!partial)
>  			folio_mark_uptodate(folio);
>  	}
> -	if (!verity)
> +	if (copied && !verity)
>  		size_changed = ext4_update_inode_size(inode, pos + copied);
>  	EXT4_I(inode)->i_datasync_tid = handle->h_transaction->t_tid;
>  	folio_unlock(folio);
>  	folio_put(folio);
>  
> -	if (old_size < pos && !verity)
> +	if (copied && old_size < pos && !verity)
>  		pagecache_isize_extended(inode, old_size, pos);
>  
>  	if (size_changed) {
> @@ -3349,6 +3352,24 @@ static int ext4_da_should_update_i_disksize(struct folio *folio,
>  	return 1;
>  }
>  
> +/*
> + * Delalloc counterpart of ext4_truncate_failed_write(): ->write_begin() only
> + * reserved delayed blocks, so drop the page cache and the delayed extents
> + * beyond i_size and leave the blocks that are allocated there alone.
> + */
> +static void ext4_da_truncate_failed_write(struct inode *inode)
> +{
> +	struct address_space *mapping = inode->i_mapping;
> +	ext4_lblk_t lblk = EXT4_B_TO_LBLK(inode, inode->i_size);
> +
> +	filemap_invalidate_lock(mapping);
> +	truncate_inode_pages(mapping, inode->i_size);
> +	down_write(&EXT4_I(inode)->i_data_sem);
> +	ext4_es_remove_extent(inode, lblk, EXT_MAX_BLOCKS - lblk);
> +	up_write(&EXT4_I(inode)->i_data_sem);
> +	filemap_invalidate_unlock(mapping);
> +}
> +
>  static int ext4_da_do_write_end(struct address_space *mapping,
>  			loff_t pos, unsigned len, unsigned copied,
>  			struct folio *folio)
> @@ -3386,12 +3407,12 @@ static int ext4_da_do_write_end(struct address_space *mapping,
>  	 * checked, we need to update i_disksize here as certain
>  	 * ext4_writepages() paths not allocating blocks and update i_disksize.
>  	 */
> -	if (new_i_size > inode->i_size) {
> +	if (copied && new_i_size > inode->i_size) {
>  		unsigned long end;
>  
>  		i_size_write(inode, new_i_size);
>  		end = offset_in_folio(folio, new_i_size - 1);
> -		if (copied && ext4_da_should_update_i_disksize(folio, end)) {
> +		if (ext4_da_should_update_i_disksize(folio, end)) {
>  			ext4_update_i_disksize(inode, new_i_size);
>  			disksize_changed = true;
>  		}
> @@ -3400,9 +3421,13 @@ static int ext4_da_do_write_end(struct address_space *mapping,
>  	folio_unlock(folio);
>  	folio_put(folio);
>  
> -	if (pos > old_size)
> +	if (copied && pos > old_size)
>  		pagecache_isize_extended(inode, old_size, pos);
>  
> +	/* Drop what ->write_begin() prepared beyond the resulting i_size. */
> +	if (pos + len > inode->i_size)
> +		ext4_da_truncate_failed_write(inode);
> +
>  	if (!disksize_changed)
>  		return copied;
>  
> 
> base-commit: 8c4f6e3376348fc3162eed86a05b933921c8f9ed