Re: [PATCH v5] 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:
- [Critical] Silent data loss when ext4_ext_insert_extent() fails with errors like -ENOMEM or -EIO.
- [High] Zero-initialization of not_found causes out-of-bounds memory operations on uninitialized xattr space.
--

commit 5924a3556bc680106bf975144b0fa43d647b3a95
Author: shuo chen <1289151713@qq.com>

    ext4: rewrite ext4_convert_inline_data_nolock to make it safer
    
    Fix issues raised by sashiko-bot.

> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index ceee69a66482b..8c589edaeff78 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
[ ... ]
> @@ -1079,21 +1080,120 @@ 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)
>  {
> -	int ret;
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	struct ext4_xattr_ibody_find is = {
> +		.s = { .not_found = 0, },
> +	};

[Severity: High]
Does initializing .not_found to 0 set up a false positive match instead of
the expected -ENODATA?

If a filesystem image is mounted where a file has EXT4_INODE_INLINE_DATA
set but lacks the EXT4_STATE_XATTR flag, ext4_xattr_ibody_find() will exit
early without modifying is.s.not_found.

Because it was initialized to 0 (meaning 'found'), later in this function:

    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);

The code believes a valid xattr entry exists at is.s.here (which points to
uninitialized space). This causes ext4_xattr_ibody_set() to attempt to
delete a bogus entry, potentially leading to out-of-bounds memory
operations and a kernel panic.

[ ... ]
> +		struct ext4_extent newex;
> +
> +		newex.ee_block = cpu_to_le32(0);
> +		newex.ee_len = cpu_to_le16(1);
> +		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);
> +		}

[Severity: Critical]
Does this restricted error check silently swallow fatal failure codes
(like -ENOMEM or -EIO) from ext4_ext_insert_extent()?

If ext4_ext_insert_extent() fails due to memory pressure or I/O error, the
restricted check allows the failure to fall through.

The unhandled error code is then overwritten shortly after:

    error = ext4_xattr_ibody_find(inode, &i, &is);

This completely masks the prior failure, causing the function to complete
successfully. The inline data flag is cleared, but the newly allocated
block is never added to the file's extent tree, permanently losing the
file's data and leaking the block.

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