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@xxxxxxxxxxxxxxx
> Signed-off-by: Alan <3164729207@xxxxxx>

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@xxxxxxxx>
SUSE Labs, CR