Re: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Data blocks are permanently leaked in the block bitmap when `ext4_ext_insert_extent` fails with an error other than EDQUOT or ENOSPC.
- [High] The extent insertion hardcodes a length of 1, breaking bigalloc support and leaking blocks if the cluster size is greater than the block size.
- [High] Error recovery path triggers a BUG_ON(count > 1) in ext4_free_blocks for regular files or bigalloc clusters.
- [High] Raw inode memory is reverted on error without marking the buffer dirty, leading to an inconsistent journal state.
- [High] The on-disk inode's size is not updated when converting an inline directory, leaving the directory with a truncated size on disk.
- [Medium] The patch implementation completely contradicts the commit message.
--

--- Patch [1]: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir ---
commit 60b81af153ab511900118d63a628e53fb1229564
Author: pipishuo <[email protected]>
Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

This commit refactors inline data conversion and adds error recovery for
extent insertion during block allocation.

> Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[Severity: Medium]
The commit message claims to add a check in ext4_readdir, but the diff
completely ignores ext4_readdir and instead refactors inline data
conversion in ext4_set_inline_data_block. Can the commit message be
updated to reflect the actual changes?

> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index ceee69a66482b..259a6af13b0f6 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
[ ... ]
> @@ -1079,21 +1080,115 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
[ ... ]
> +		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 break bigalloc support? If the
cluster size is greater than the block size, or if multiple blocks are
allocated, the len parameter is ignored. Will the additional allocated
blocks marked in the bitmap be permanently leaked since they aren't
added to the extent tree?

> +		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_forget(handle, 0, inode, bh, block);
> +				goto nofree;

[Severity: High]
If ext4_ext_insert_extent() fails with an error other than EDQUOT or
ENOSPC, the code jumps to nofree. Does this permanently leak the
allocated data blocks in the block bitmap, since the ext4_free_blocks()
call in the recovery block is bypassed?

> +			}
> +		} else {
> +			ext4_free_ext_path(path);
> +		}
[ ... ]
> +recovery:
> +	if (error) {
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
> +nofree:
> +		memcpy((void *)raw_inode, inode_buf, inode_size);
> +		memcpy(ei->i_data, raw_inode->i_block, EXT4_MIN_INLINE_DATA_SIZE);
> +		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);

[Severity: High]
This reverts the raw_inode memory on error, but is the buffer ever
marked dirty again? Because ext4_ext_tree_init() previously marked the
buffer dirty in the active transaction, doesn't overwriting the memory
here without calling ext4_mark_iloc_dirty() lead to an inconsistent
journal state?

> +	} else {
> +		ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
> +		get_bh(is.iloc.bh);
> +		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
> +		EXT4_I(inode)->i_inline_off = 0;
> +		EXT4_I(inode)->i_inline_size = 0;
> +		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
> +		if (S_ISDIR(inode->i_mode)) {
> +			i_size_write(inode, inode->i_sb->s_blocksize);
> +			EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
> +		}

[Severity: High]
Because ext4_mark_iloc_dirty() is called before the S_ISDIR size
updates, does this cause the new size to be missed in the buffer update?
Will the on-disk inode retain the old truncated inline data size?

> +		brelse(bh);
> +	}
[ ... ]
> @@ -1103,8 +1198,11 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
[ ... ]
> +out_bh:
> +	if (error) {
> +		int flags = 0;
>  
> -out_restore:
> -	if (error)
> -		ext4_restore_inline_data(handle, inode, iloc, buf, inline_size);
> +		if (data_bh)
> +			flags |= EXT4_FREE_BLOCKS_FORGET;
> +		struct ext4_inode_info *ei = EXT4_I(inode);
>  
> +		down_write(&ei->i_data_sem);
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, data_bh, newblock, allocated_block, flags);

[Severity: High]
Does this trigger a BUG_ON in ext4_free_blocks() on error paths? If we
allocate multiple blocks (e.g., when bigalloc is enabled or
EXT4_MB_HINT_DATA is used), allocated_block can be > 1.

Calling ext4_free_blocks() with a non-NULL data_bh, a count > 1, and the
EXT4_FREE_BLOCKS_FORGET flag will hit the BUG_ON(count > 1) in
ext4_free_blocks().

> +		up_write(&ei->i_data_sem);
> +	} else {
> +		error = ext4_set_inline_data_block(handle, inode,
> +				newblock, allocated_block, data_bh);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1