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

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

[Hengyu Liang]

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>
---
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
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: broken inline data conversion in ext4_da_truncate_failed_write()
--

commit c64fed07a736168af76e10e5cf487bb520de68ee
Author: Hengyu Liang <hengyul@cs.unc.edu>

ext4: do not advance i_size on a buffered write that copied nothing

This commit prevents ext4_write_end(), ext4_journalled_write_end(), and
ext4_da_do_write_end() from advancing i_size when a buffered write copies
zero bytes. In the delayed allocation path, it also adds
ext4_da_truncate_failed_write() to drop page cache folios and delayed
extents prepared beyond i_size after a failed or short write.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6ede3644a738b..9c20876629bc1 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c

[ ... ]

> @@ -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);

[Severity: High]
Can calling ext4_da_truncate_failed_write() when inode->i_size == 0 leave an
inline-data inode in a half-converted state?

Suppose an inode has inline data initialized (EXT4_INODE_INLINE_DATA is set,
EXT4_INODE_EXTENTS is clear, EXT4_STATE_MAY_INLINE_DATA is set, and
i_inline_size == 60) with inode->i_size == 0 (for example, after truncating
an inline file to 0 or after a small write that failed with -EFAULT in
ext4_write_inline_data_end()).

If a user then issues a buffered write with pos + len exceeding
ext4_get_max_inline_size(inode) that copies 0 bytes, ext4_da_write_begin()
calls ext4_generic_write_inline_data() ->
ext4_da_convert_inline_data_to_extent():

fs/ext4/inline.c:ext4_da_convert_inline_data_to_extent() {
	...
	ret = ext4_block_write_begin(NULL, folio, 0, inline_size,
				     ext4_da_get_block_prep);
	...
	clear_buffer_new(folio_buffers(folio));
	folio_mark_dirty(folio);
	folio_mark_uptodate(folio);
	ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
	...
}

This instantiates folio 0, reserves a delayed block at lblk 0, marks folio 0
dirty, and clears EXT4_STATE_MAY_INLINE_DATA while leaving
EXT4_INODE_INLINE_DATA set so that ext4_do_writepages() will later call
ext4_destroy_inline_data() during writeback.

When ext4_da_do_write_end() runs with copied == 0, inode->i_size remains 0
and ext4_da_truncate_failed_write() is called with lblk == 0. That drops
dirty folio 0 and removes the delayed extent at lblk 0, leaving the inode
with EXT4_INODE_INLINE_DATA set, EXT4_INODE_EXTENTS clear,
EXT4_STATE_MAY_INLINE_DATA clear, and no dirty folio in the page cache:

1. If the 0-byte copy happened because the user buffer was not yet faulted in
   on a write at pos >= PAGE_SIZE, generic_perform_write() faults in the user
   page and retries ext4_da_write_begin(). Because EXT4_STATE_MAY_INLINE_DATA
   is now clear, ext4_da_write_begin() skips recreating folio 0, so
   writeback will not convert the inline data unless another write touches
   folio 0.

2. If a subsequent write enters ext4_write_begin() (for example via
   ext4_nonda_switch()), it skips ext4_try_to_write_inline_data() because
   EXT4_STATE_MAY_INLINE_DATA is clear, and _ext4_get_block() fails with
   -ERANGE because ext4_has_inline_data(inode) is still true.

3. If a subsequent delalloc write writes N <= i_inline_size bytes at pos 0
   (creating dirty folio 0 with BH_Delay and a delayed extent at lblk 0
   while EXT4_INODE_INLINE_DATA is still set) and the file is then truncated
   to M (where 0 < M < N) before writeback, ext4_setattr() skips
   ext4_convert_inline_data() because M <= i_inline_size, and calls
   ext4_inline_data_truncate():

fs/ext4/inline.c:ext4_inline_data_truncate() {
	...
	if (i_size < inline_size) {
		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
			ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
	...
}

   This removes the delayed extent at lblk 0 from the extent status tree
   while folio 0 remains dirty with BH_Delay in the page cache. When
   writeback runs, ext4_map_blocks() finds no delayed extent in the extent
   status tree and returns 0 under EXT4_GET_BLOCKS_IO_SUBMIT, triggering
   WARN_ON_ONCE(!err) in mpage_map_one_extent() and discarding the dirty
   folio in mpage_map_and_submit_extent().

Should ext4_da_truncate_failed_write() restore EXT4_STATE_MAY_INLINE_DATA if
ext4_has_inline_data(inode) is still true when truncating back to 0 (or
avoid dropping folio 0 / lblk 0 when inline data conversion is pending)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030040520.4138904-1-hengyul@cs.unc.edu?part=1