Re: [PATCH v6] ext4: rewrite ext4_convert_inline_data_nolock to make it safer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Silent data block leak when ext4_ext_insert_extent fails with non-space errors.
- [High] Block leak of over-allocated blocks from ext4_mb_new_blocks() due to hardcoded extent length.
--

commit 4f5429b131a64abd614d1b0e2bd7535d3f83a7c0
Author: shuo chen <1289151713@qq.com>

ext4: rewrite ext4_convert_inline_data_nolock to make it safer

This commit refactors the inline data conversion logic to address issues with
uninitialized xattr space and data loss on extent insertion failures. It aims
to revert the inode back to inline data if extent insertion fails for reasons
other than quota or space limitations.

> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index ceee69a66482b..5a3763d3b51a5 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
[ ... ]
> @@ -1079,21 +1080,123 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
[ ... ]
> +	if (ext4_has_feature_extents(inode->i_sb) &&
> +		(S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) || S_ISLNK(inode->i_mode))) {
> +		ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
> +		ext4_ext_tree_init(handle, inode);
> +		struct ext4_ext_path *path = ext4_find_extent(inode, 0, NULL, 0);
> +
> +		if (IS_ERR(path)) {
> +			error = PTR_ERR(path);
> +			goto recovery;
> +		}
> +		struct ext4_extent newex;
> +
> +		newex.ee_block = cpu_to_le32(0);
> +		newex.ee_len = cpu_to_le16(1);
> +		ext4_ext_store_pblock(&newex, block);

[Severity: High]
Does this hardcode the extent length to 1 block, potentially leaking blocks if 
ext4_mb_new_blocks() allocates a larger contiguous extent?

In ext4_convert_inline_data_nolock(), the actual allocation size is captured
as allocated_block = EXT4_C2B(sbi, ar.len). If the allocation request returns
multiple blocks (for example, on a bigalloc filesystem), only the first block
is added to the extent tree here. The remaining blocks appear to be neither
mapped nor freed, which would leave them orphaned in the block bitmap.

> +		path = ext4_ext_insert_extent(handle, inode, path, &newex, 0);
> +		if (IS_ERR(path)) {
> +			error = PTR_ERR(path);
> +			if (error == -EDQUOT || error == -ENOSPC)
> +				goto recovery;
> +			else
> +				goto no_free;
> +		} else {
> +			ext4_free_ext_path(path);
> +		}
[ ... ]
> +recovery:
> +	if (error) {
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, NULL, block, len, 0);
> +		memcpy(header, xattr_buf, xattr_size);
> +no_free:
> +		ext4_forget(handle, 0, inode, bh, block);

[Severity: High]
Can this error path result in a permanent data block leak?

If ext4_ext_insert_extent() fails with an error like -ENOMEM or -EIO, the
code jumps directly to the no_free label. This deliberately bypasses the
ext4_free_blocks() call just above it. Because the calling function relies
on ext4_set_inline_data_block() to free the newly allocated block on error,
bypassing the free operation here appears to leave the blocks permanently
marked as used in the filesystem.

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