[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

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_9EB8EBE910378E46C80E006C6A7DA918BA08@qq.com?part=1

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] ext4: single-word name in Author and Signed-off-by trailer
--

commit 28c8d79985e366b191e9eac1f2d5415854ca0289
Author: Alan <3164729207@qq.com>

> ext4: free the new extent block on failure to grow the tree
>
> 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>

[Severity: High]
Documentation/process/submitting-patches.rst requires a full real name in
the Author header and Signed-off-by trailer to satisfy the Developer's
Certificate of Origin, rather than a single-word name or handle ("Alan").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_DF25FDEF46DA68C4E18D6DB40F08E30BB705@qq.com?part=1

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

[Jan Kara]

On Wed 11-11-26 15:34:44, Alan wrote:
> 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>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR