[PATCH] ext4: free the new extent block when getting its buffer fails

[PATCH] ext4: free the new extent block when getting its buffer fails

[Alan]

ext4_ext_grow_indepth() allocates and accounts a metadata block before
calling sb_getblk_gfp(). If the buffer allocation fails, the new block
has not been linked into the extent tree, but the error path returns
without freeing it. This leaks the block and leaves i_blocks larger
than the number of blocks reachable from the inode.

Commit 8a83ac54940d ("ext4: call bdev_getblk() from sb_getblk_gfp()")
removed the implicit __GFP_NOFAIL from this allocation, making this
error path reachable.

Free the new metadata block before returning -ENOMEM. Use
ext4_free_blocks() to undo both the allocation and inode accounting.

Tested on Linux 6.12.1 by injecting a NULL result at the caller's buffer
acquisition boundary. Before the fix, seven failed extent tree growth
operations left seven allocated but unreachable blocks and i_blocks of
40 instead of 32 on a filesystem with 4 KiB blocks. After the fix, the
same failures leave i_blocks at 32, and e2fsck -fn passes after a normal
unmount. Retrying the operation, truncating the file, and growing the
tree without injection also succeed.

Fixes: 8a83ac54940d ("ext4: call bdev_getblk() from sb_getblk_gfp()")
Cc: stable@vger.kernel.org
Signed-off-by: Alan <3164729207@qq.com>
---
 fs/ext4/extents.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ad0d80e33f6090..7358df54bcd4ae 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1333,8 +1333,12 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
 		return err;
 
 	bh = sb_getblk_gfp(inode->i_sb, newblock, __GFP_MOVABLE | GFP_NOFS);
-	if (unlikely(!bh))
+	if (unlikely(!bh)) {
+		/* The new block has not been linked into the extent tree yet. */
+		ext4_free_blocks(handle, inode, NULL, newblock, 1,
+				 EXT4_FREE_BLOCKS_METADATA);
 		return -ENOMEM;
+	}
 	lock_buffer(bh);
 
 	err = ext4_journal_get_create_access(handle, inode->i_sb, bh,

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.53.0

Re: [PATCH] ext4: free the new extent block when getting its buffer fails

[Jan Kara]

On Tue 10-11-26 15:27:23, Alan wrote:
> ext4_ext_grow_indepth() allocates and accounts a metadata block before
> calling sb_getblk_gfp(). If the buffer allocation fails, the new block
> has not been linked into the extent tree, but the error path returns
> without freeing it. This leaks the block and leaves i_blocks larger
> than the number of blocks reachable from the inode.
> 
> Commit 8a83ac54940d ("ext4: call bdev_getblk() from sb_getblk_gfp()")
> removed the implicit __GFP_NOFAIL from this allocation, making this
> error path reachable.
> 
> Free the new metadata block before returning -ENOMEM. Use
> ext4_free_blocks() to undo both the allocation and inode accounting.
> 
> Tested on Linux 6.12.1 by injecting a NULL result at the caller's buffer
> acquisition boundary. Before the fix, seven failed extent tree growth
> operations left seven allocated but unreachable blocks and i_blocks of
> 40 instead of 32 on a filesystem with 4 KiB blocks. After the fix, the
> same failures leave i_blocks at 32, and e2fsck -fn passes after a normal
> unmount. Retrying the operation, truncating the file, and growing the
> tree without injection also succeed.
> 
> Fixes: 8a83ac54940d ("ext4: call bdev_getblk() from sb_getblk_gfp()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alan <3164729207@qq.com>

OK, but this holds for any error in that function at least until we call
ext4_idx_store_pblock(). So please fix at least all occurences in that one
function. Thanks.

								Honza

> ---
>  fs/ext4/extents.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index ad0d80e33f6090..7358df54bcd4ae 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1333,8 +1333,12 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
>  		return err;
>  
>  	bh = sb_getblk_gfp(inode->i_sb, newblock, __GFP_MOVABLE | GFP_NOFS);
> -	if (unlikely(!bh))
> +	if (unlikely(!bh)) {
> +		/* The new block has not been linked into the extent tree yet. */
> +		ext4_free_blocks(handle, inode, NULL, newblock, 1,
> +				 EXT4_FREE_BLOCKS_METADATA);
>  		return -ENOMEM;
> +	}
>  	lock_buffer(bh);
>  
>  	err = ext4_journal_get_create_access(handle, inode->i_sb, bh,
> 
> base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2] ext4: free the new extent block on failure to grow the tree

[Alan]

ext4_ext_grow_indepth() allocates and accounts a metadata block before
getting its buffer, obtaining journal create access and dirtying the
new extent block. If any of these operations fails, the new block has
not been linked into the extent tree. However, the error paths return
without freeing it, leaking the block and leaving i_blocks larger
than the number of blocks reachable from the inode.

Commit 8a83ac54940d ("ext4: call bdev_getblk() from sb_getblk_gfp()")
removed the implicit __GFP_NOFAIL from the buffer allocation, making
the allocation failure path reachable. The two journal error paths
already lacked cleanup before that commit and are fixed here as well.

Route all three failures through a common cleanup label to free the
unpublished metadata block and undo its inode accounting. Keep the
existing buffer unlock and release handling. Once the new block has
been linked by ext4_idx_store_pblock(), an ext4_mark_inode_dirty()
failure must not free it because the extent tree already references it.

Tested on Linux 6.12.1 by injecting NULL for buffer acquisition and
-EIO for each of the two metadata helper calls at their caller
boundaries. Each unfixed case leaves seven unreachable blocks and
i_blocks of 40 instead of 32 on a filesystem with 4 KiB blocks. With
the fix, all three cases retain i_blocks of 32, preserve the original
data and pass e2fsck -fn after a normal unmount. Retry, truncate and
uninjected growth also pass for each case.

Fixes: 8a83ac54940d ("ext4: call bdev_getblk() from sb_getblk_gfp()")
Cc: stable@vger.kernel.org
Signed-off-by: Alan <3164729207@qq.com>
---
 fs/ext4/extents.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ad0d80e33f6090..f0b2852f8a9342 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1333,15 +1333,17 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
 		return err;
 
 	bh = sb_getblk_gfp(inode->i_sb, newblock, __GFP_MOVABLE | GFP_NOFS);
-	if (unlikely(!bh))
-		return -ENOMEM;
+	if (unlikely(!bh)) {
+		err = -ENOMEM;
+		goto out_free;
+	}
 	lock_buffer(bh);
 
 	err = ext4_journal_get_create_access(handle, inode->i_sb, bh,
 					     EXT4_JTR_NONE);
 	if (err) {
 		unlock_buffer(bh);
-		goto out;
+		goto out_free;
 	}
 
 	ext_size = sizeof(EXT4_I(inode)->i_data);
@@ -1366,7 +1368,7 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
 
 	err = ext4_handle_dirty_metadata(handle, inode, bh);
 	if (err)
-		goto out;
+		goto out_free;
 
 	/* Update top-level index: num,max,pointer */
 	neh = ext_inode_hdr(inode);
@@ -1389,6 +1391,12 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
 	brelse(bh);
 
 	return err;
+
+out_free:
+	/* The new block has not been linked into the extent tree yet. */
+	ext4_free_blocks(handle, inode, NULL, newblock, 1,
+			 EXT4_FREE_BLOCKS_METADATA);
+	goto out;
 }
 
 /*

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.53.0