Re: [PATCH V3] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Misordered metadata update causes corrupt directory `i_size` on disk.
- [Critical] Unconditional BUG_ON() crash in `ext4_free_blocks()` on error paths for allocations > 1 block.
- [High] Permanent block leak when `ext4_ext_insert_extent` fails with an error other than EDQUOT or ENOSPC.
- [High] Concurrent inode metadata changes are overwritten during error recovery.
- [High] Permanent space leak due to hardcoded extent length of 1 block.
- [High] Missing `i_blocks` accounting for newly allocated physical blocks.
- [Medium] The commit message describes a completely different change.
--

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

The commit message describes adding a state check in ext4_readdir, but the
patch refactors ext4_convert_inline_data_nolock and introduces a new block
allocation mechanism for inline data conversion.

>     Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir
>     
>     Resend as v3 because I forgot to add the version number

[Severity: Medium]
Is the commit message mismatched with the actual code changes? The subject
and message describe adding a state check in ext4_readdir, but the patch
entirely refactors ext4_convert_inline_data_nolock and introduces a new
block allocation mechanism for inline data conversion without mentioning it.

> --- 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,
>  	return 0;
>  }
>  
> -static void ext4_restore_inline_data(handle_t *handle, struct inode *inode,
> -				     struct ext4_iloc *iloc,
> -				     void *buf, int inline_size)
> +static int ext4_set_inline_data_block(handle_t *handle, struct inode *inode, ext4_fsblk_t block,
> +					unsigned int len, struct buffer_head *bh)
>  {
[ ... ]
> +		struct ext4_extent newex;
> +
> +		newex.ee_block = cpu_to_le32(0);
> +		newex.ee_len = cpu_to_le16(1);

[Severity: High]
Does this permanently leak physical blocks if the allocator returns more
than 1 block? When ext4_mb_new_blocks() returns a larger allocation
(e.g., due to cluster normalization on bigalloc filesystems), hardcoding
the extent length to 1 abandons the extra blocks. Should this use the
allocated length (len parameter) instead?

> +		ext4_ext_store_pblock(&newex, block);
> +		path = ext4_ext_insert_extent(handle, inode, path, &newex, 0);

[Severity: High]
Does this miss accounting for the newly allocated physical blocks?
Manually inserting the extent bypasses the normal ext4_map_blocks()
pipeline, meaning ext4_inode_add_physical_space() is never called to
increment inode->i_blocks. Could this lead to stat() inconsistencies and
fsck failures?

> +		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;
> +			}
> +		} 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);
> +	if (error)
> +		goto recovery;
> +	if (!is.s.not_found)
> +		error = ext4_xattr_ibody_set(handle, inode, &i, &is);
> +recovery:
> +	if (error) {
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);

[Severity: Critical]
Can this trigger a BUG_ON() crash in ext4_free_blocks() on bigalloc
filesystems? If the length (len) is greater than 1, calling
ext4_free_blocks() with both a valid buffer_head (bh) and the
EXT4_FREE_BLOCKS_FORGET flag will unconditionally hit the
BUG_ON(count > 1) check inside ext4_free_blocks().

> +nofree:

[Severity: High]
Does the jump to the nofree label earlier permanently leak the allocated
blocks if the extent insertion fails with an error other than -EDQUOT or
-ENOSPC? Jumping here bypasses the ext4_free_blocks() call during error
recovery. Because ext4_forget() only removes the buffer from the journal
without freeing the physical blocks in the buddy bitmap, the blocks appear
to be permanently leaked.

> +		memcpy((void *)raw_inode, inode_buf, inode_size);

[Severity: High]
Could this non-atomic memcpy() overwrite concurrent metadata updates?
The i_data_sem protects the block mapping, but background flushers or
other operations might have updated the inode's timestamps or extended
attributes in the raw_inode while we were allocating blocks. Blindly
copying back the old inode_buf could silently discard those changes.

> +		memcpy(ei->i_data, raw_inode->i_block, EXT4_MIN_INLINE_DATA_SIZE);
> +		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
> +	} 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: Critical]
Does this permanently save a stale directory size to disk?
The raw metadata buffer is marked dirty via ext4_mark_iloc_dirty()
before the VFS inode size and i_disksize are updated. Since this patch
removed the later ext4_mark_inode_dirty() call, is there a risk the
updated size fields never make it to the on-disk raw_inode?

> +		brelse(bh);
> +	}
[ ... ]
> @@ -1168,24 +1263,28 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
>  						   inode, data_bh);
>  	} else {
>  		unlock_buffer(data_bh);
> -		inode->i_size = inode->i_sb->s_blocksize;
> -		i_size_write(inode, inode->i_sb->s_blocksize);
> -		EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
> -
>  		error = ext4_init_dirblock(handle, inode, data_bh,
>  			  le32_to_cpu(((struct ext4_dir_entry_2 *)buf)->inode),
>  			  buf + EXT4_INLINE_DOTDOT_SIZE,
>  			  inline_size - EXT4_INLINE_DOTDOT_SIZE);
> -		if (!error)
> -			error = ext4_mark_inode_dirty(handle, inode);
>  	}
> +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: Critical]
Similarly, could this also trigger the BUG_ON(count > 1) in
ext4_free_blocks() on failure? If allocated_block is greater than 1
(e.g., bigalloc) and data_bh is valid, flags will include
EXT4_FREE_BLOCKS_FORGET, which crashes when freeing multiple blocks
with a provided buffer_head.

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