Re: [PATCH v2] ext4: validate extent root during fast commit replay

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ext4: false corruption errors in ext4_find_extent() on
  non-extent replay
- [Medium] ext4: inodes cached during fast commit replay bypass
  ext4_find_extent() check
--

commit adf8509abc940e5e79c96840165b645b5c4cea05
Author: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>

ext4: validate extent root during fast commit replay

This commit validates the inode extent header via ext4_ext_check() in
ext4_find_extent() when EXT4_FC_REPLAY is set in s_mount_state.

> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 7193429a05e1f..c275bb23658e3 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -905,6 +905,12 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>  		ret = -EFSCORRUPTED;
>  		goto err;
>  	}
> +	/* ext4_iget() skips extent validation during fast commit replay. */
> +	if (unlikely(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)) {
> +		ret = ext4_ext_check(inode, eh, depth, 0);
> +		if (ret)
> +			goto err;
> +	}

[Severity: High]
Does running ext4_ext_check() here when EXT4_INODE_EXTENTS is clear trigger
false-positive filesystem corruption errors when replaying valid non-extent
inodes?

Special files created via ext4_mknod() (FIFOs, sockets, device nodes) and
fast symlinks do not set EXT4_INODE_EXTENTS or EXT4_INODE_INLINE_DATA, and
their i_data holds zeroed bytes, rdev, or the symlink target rather than an
ext4_extent_header.  They are still tracked by fast commit via
ext4_fc_track_create() and ext4_fc_track_inode().

During recovery, ext4_fc_replay_inode() calls ext4_ext_clear_bb() and
ext4_ext_replay_set_iblocks() whenever EXT4_INODE_INLINE_DATA is clear, and
both call ext4_find_extent():

fs/ext4/fast_commit.c:ext4_fc_replay_inode() {
	...
	if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA))
		ext4_ext_replay_set_iblocks(inode);
	...
}

Previously, ext4_find_extent() saw depth == 0 and eh_entries == 0 for a
special inode and returned an empty path, which those callers treated as a
no-op.  With this regression, ext4_ext_check() sees eh_magic !=
EXT4_EXT_MAGIC and calls ext4_error_inode_err() -> ext4_handle_error(),
aborting replay, marking the filesystem corrupted, and remounting read-only
or panicking the kernel.

[Severity: Medium]
Can an inode loaded by ext4_iget() during fast commit replay stay cached in
memory and bypass extent root validation after EXT4_FC_REPLAY is cleared?

During replay, __ext4_iget() skips ext4_ext_check_inode() and inserts the
inode into the VFS cache.  Replay paths such as ext4_fc_replay_link() and
ext4_fc_replay_create() call ext4_iget() and iput() without calling
ext4_find_extent(), and ext4_fc_replay_inode() ignores the return values of
ext4_ext_clear_bb() and ext4_ext_replay_set_iblocks() without evicting the
inode.

Once ext4_fc_replay_cleanup() clears EXT4_FC_REPLAY, subsequent ext4_iget()
calls hit the cached inode (I_NEW is clear) and return it without running
ext4_ext_check_inode(), while later ext4_find_extent() calls skip this new
check because EXT4_FC_REPLAY is no longer set.  A corrupt root header on
that cached inode can then still overflow i_data in
ext4_ext_insert_extent() after mount completes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108201906.1417129-2-Jeremy.Jean@oss.cyber.gouv.fr?part=1