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

[PATCH v2] 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.  The delalloc path does not
trim after a failed write, so when such a write reaches past i_size,
it also drops the folio and the delayed blocks that ->write_begin()
set up there.  Otherwise that folio stays dirty past EOF and writeback
never cleans it.

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 v2:
- Rebase onto the ext4 dev branch.
- Also trim when a failed write starts before EOF and ends past it
  (pos + len > old_size), as suggested by Zhang Yi.
- Keep the Fixes tag and the new helper, as discussed on v1.
- v1: https://lore.kernel.org/r/20261009044656.1993196-1-hengyul@cs.unc.edu

Tested in KVM with KASAN: the reproducer above and failed writes around
EOF on delalloc, nodelalloc, bigalloc and 1k block filesystems.
e2fsck -fn reports no errors.

 fs/ext4/inode.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 6ede3644a738..e96321ce17d2 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,16 @@ 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);
 
+	/*
+	 * A write reaching past i_size that copied nothing leaves i_size
+	 * alone, so trim what ->write_begin() set up beyond it.
+	 */
+	if (!copied && pos + len > old_size)
+		ext4_da_truncate_failed_write(inode);
+
 	if (!disksize_changed)
 		return copied;
 

base-commit: 8c4f6e3376348fc3162eed86a05b933921c8f9ed
-- 
2.53.0

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

[Zhang Yi]

On 10/24/2026 1:25 AM, 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.  The delalloc path does not
> trim after a failed write, so when such a write reaches past i_size,
> it also drops the folio and the delayed blocks that ->write_begin()
> set up there.  Otherwise that folio stays dirty past EOF and writeback
> never cleans it.

I feel the last sentence isn't quite accurate. Dirty folio past
EOF will be cleaned up during writeback. In mpage_process_page_bufs(),
when lblk >= blocks, if 1) there are no blocks to map, it will
directly call mpage_submit_folio(), which clears the dirty flag on
the folio, and then mpage_submit_folio() will shorten the writeback
length, so that ext4_bio_write_folio() will also clear the dirty
flag on the corresponding bh; if 2) there are blocks to map, then
after the required block allocation is done, it will likewise call
mpage_submit_folio() and do similar cleanup. Am I missing something?

> 
> 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 v2:
> - Rebase onto the ext4 dev branch.
> - Also trim when a failed write starts before EOF and ends past it
>    (pos + len > old_size), as suggested by Zhang Yi.
> - Keep the Fixes tag and the new helper, as discussed on v1.
> - v1: https://lore.kernel.org/r/20261009044656.1993196-1-hengyul@cs.unc.edu
> 
> Tested in KVM with KASAN: the reproducer above and failed writes around
> EOF on delalloc, nodelalloc, bigalloc and 1k block filesystems.
> e2fsck -fn reports no errors.
> 
>   fs/ext4/inode.c | 42 +++++++++++++++++++++++++++++++++++-------
>   1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6ede3644a738..e96321ce17d2 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,16 @@ 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);
>   
> +	/*
> +	 * A write reaching past i_size that copied nothing leaves i_size
> +	 * alone, so trim what ->write_begin() set up beyond it.
> +	 */
> +	if (!copied && pos + len > old_size)

Why do you keep the '!copied' condition? AFAICT, if copied is non-zero
but less than len, the excess delalloc extent allocate by
ext4_da_write_begin() should still be cleaned up.

Thanks,
Yi.

> +		ext4_da_truncate_failed_write(inode);
> +
>   	if (!disksize_changed)
>   		return copied;
>   
> 
> base-commit: 8c4f6e3376348fc3162eed86a05b933921c8f9ed