Re: [PATCH 2/3] ext4: fix directory i_blocks after fast commit replay

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] ext4: missing EXT4_INODE_EXTENTS check before
  ext4_ext_replay_set_iblocks()
- [High] ext4: ext4_ext_replay_set_iblocks() drops directory xattr
  blocks in i_blocks
- [Medium] ext4: skipped dir i_blocks recount on -EEXIST in
  ext4_fc_replay_link_internal()
--

commit bd9227cad3ab9ec91535ca3b97b88447eed97729
Author: Mani Tofigh <mt4022@columbia.edu>

ext4: fix directory i_blocks after fast commit replay

This commit recounts i_blocks via ext4_ext_replay_set_iblocks() for newly
initialized directories and parent directories modified during fast commit
replay.

> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 0eac7be3f58c2..9c170986f0b21 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -1858,6 +1858,9 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
>  		goto out;
>  	}
>  
> +	/* The replay allocator doesn't count blocks added to the dir */
> +	if (!ret && !ext4_test_inode_flag(dir, EXT4_INODE_INLINE_DATA))
> +		ext4_ext_replay_set_iblocks(dir);

[Severity: High]
Does calling ext4_ext_replay_set_iblocks(dir) lose external xattr block
accounting on dir->i_blocks?

ext4_ext_replay_set_iblocks() overwrites inode->i_blocks with only the
extent data and tree block count:

fs/ext4/extents.c:ext4_ext_replay_set_iblocks() {
    ...
out:
	inode->i_blocks = numblks << (inode->i_sb->s_blocksize_bits - 9);
	ext4_mark_inode_dirty(NULL, inode);
    ...
}

If dir already has an external xattr block (EXT4_I(dir)->i_file_acl) or
ea_inode blocks from an earlier transaction, replaying any link or file
creation in dir drops those blocks from dir->i_blocks.  Besides corrupting
on-disk i_blocks, later removing the directory or its xattrs frees
i_file_acl and underflows dir->i_blocks.

[Severity: Medium]
Does gating ext4_ext_replay_set_iblocks(dir) on !ret skip the i_blocks
recount if a previous replay crashed after __ext4_link() persisted the new
directory block and dirent, when the re-run of __ext4_link() returns
-EEXIST?

>  	ret = 0;
>  out:
>  	if (dir)
> @@ -2070,6 +2073,9 @@ static int ext4_fc_replay_create(struct super_block *sb,
>  			ret = 0;
>  			goto out;
>  		}
> +		/* The replay allocator doesn't count the new dir block */
> +		if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA))
> +			ext4_ext_replay_set_iblocks(inode);

[Severity: High]
Can this (and the matching check in ext4_fc_replay_link_internal()) call
ext4_ext_replay_set_iblocks() on an indirect-mapped directory that does not
have EXT4_INODE_EXTENTS set?

A directory will lack EXT4_INODE_EXTENTS if the filesystem was formatted
without the extents feature or if EXT4_EXTENTS_FL was cleared on the
directory via FS_IOC_SETFLAGS in an earlier transaction.  During fast
commit replay, __ext4_iget() skips extent header validation, and
ext4_ext_replay_set_iblocks() calls ext4_find_extent(..., EXT4_EX_NOCACHE),
which reinterprets the direct block numbers in EXT4_I(inode)->i_data[] as a
struct ext4_extent_header and binary-searches past i_data[] into adjacent
slab memory or corrupts i_blocks.

Checking ext4_test_inode_flag(..., EXT4_INODE_EXTENTS) as
ext4_fc_replay_inode() does would avoid calling into the extent tree
walker on non-extent directories.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-ext4-fc-replay-mkdir-v1-0-4b20be29aed8@columbia.edu?part=2