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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Silent data loss, block leak, and file corruption when `ext4_ext_insert_extent` fails with non-quota errors.
- [Critical] Incomplete error rollback leaves the on-disk inode corrupted with EXT4_INODE_EXTENTS flag set but inline data present in i_block.
- [High] Buffer head reference leak on the xattr_buf allocation error path.
- [High] Hardcoded extent length to 1 leaks extra allocated blocks when ext4_mb_new_blocks returns a larger extent.
--

commit ce8b20336077e6f612c3df128b34228b4e54e5b5
Author: shuo chen <1289151713@qq.com>
ext4: rewrite ext4_convert_inline_data_nolock to make it safer

This patch rewrites ext4_convert_inline_data_nolock() by preallocating
blocks and handling errors more robustly. It also introduces
ext4_set_inline_data_block() to manage the conversion of inline data to blocks.

> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index ceee69a66482b..d1cf3c702e79e 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
[ ... ]
> @@ -1079,21 +1080,125 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
[ ... ]
> +	error = ext4_get_inode_loc(inode, &is.iloc);
> +	if (error)
> +		goto error;
> +	raw_inode = ext4_raw_inode(&is.iloc);
> +	header = IHDR(inode, raw_inode);
> +	tail = ITAIL(inode, raw_inode);
> +	xattr_size = tail - header;
> +	xattr_buf = kmalloc(xattr_size, GFP_NOFS);
> +	if (!xattr_buf) {
> +		error = -ENOMEM;
> +		goto error;
> +	}

[Severity: High]
Does this error path leak a reference to is.iloc.bh?

If kmalloc() for xattr_buf fails, the code jumps to the error label which does
not call brelse(is.iloc.bh). Since ext4_get_inode_loc() pins a reference
on is.iloc.bh, could this result in a permanent buffer head reference leak?

[ ... ]
> +	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);

[Severity: High]
Does hardcoding the extent length to 1 leak extra allocated blocks?

In ext4_convert_inline_data_nolock(), ext4_mb_new_blocks() may return a
larger extent due to bigalloc or normalization, passing the total allocated
blocks as len:

fs/ext4/inline.c:ext4_convert_inline_data_nolock() {
	...
	newblock = ext4_mb_new_blocks(handle, &ar, &error);
	if (error < 0)
		goto out;
	allocated_block = EXT4_C2B(sbi, ar.len);
	...
		error = ext4_set_inline_data_block(handle, inode,
				newblock, allocated_block, bh);
	...
}

If multiple blocks were allocated, the remaining blocks might be permanently
leaked since they are marked as used but not mapped to the file.

> +		ext4_ext_store_pblock(&newex, block);
> +		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 {
> +			ext4_free_ext_path(path);
> +		}
> +	} else {
> +		EXT4_I(inode)->i_data[0] = cpu_to_le32(block);
> +	}
> +	error = ext4_xattr_ibody_find(inode, &i, &is);

[Severity: Critical]
Is it possible this overwrites the error code from ext4_ext_insert_extent()?

If ext4_ext_insert_extent() fails with an error other than -EDQUOT or
-ENOSPC, execution falls through to the ext4_xattr_ibody_find() call, which
could succeed (returning 0) and overwrite the failure. This would cause the
function to proceed to the success path, clearing EXT4_INODE_INLINE_DATA
without actually inserting the extent, potentially leading to data loss and
a block leak.

> +	if (error)
> +		goto recovery;
> +	if (!is.s.not_found)
> +		error = ext4_xattr_ibody_set(handle, inode, &i, &is);
> +recovery:
> +	if (error) {
> +		ext4_forget(handle, 0, inode, bh, block);
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, NULL, block, len, 0);
> +		memcpy((void *)raw_inode->i_block, i_block_buf, EXT4_MIN_INLINE_DATA_SIZE);
> +		memcpy(ei->i_data, raw_inode->i_block, EXT4_MIN_INLINE_DATA_SIZE);
> +		memcpy(header, xattr_buf, xattr_size);
> +		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);

[Severity: Critical]
Does this recovery path miss a call to ext4_mark_iloc_dirty()?

The code restores the inline data into raw_inode->i_block and clears the
EXT4_INODE_EXTENTS flag, but doesn't mark the inode dirty. Since
ext4_ext_tree_init() previously dirtied the inode, it might be written to
disk with an invalid checksum and the EXT4_EXTENTS_FL flag still set in
raw_inode->i_flags, which could cause metadata corruption.

> +	} else {
> +		ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
[ ... ]

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