Re: [PATCH 1/3] ext4: fix directory count after fast commit replay

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ext4: unaligned memory access on raw_fc_inode in
  ext4_fc_replay_inode()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] ext4: unpropagated parent lookup error in
  ext4_fc_replay_link_internal() [1]
--

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

ext4: fix directory count after fast commit replay

This commit passes the inode mode to ext4_mark_inode_used() so that fast
commit replay increments bg_used_dirs_count when marking a directory inode
in use, and moves the ext4_iget() lookup in ext4_fc_replay_create() ahead
of ext4_mark_inode_used().

> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 0cac890cf3702..0eac7be3f58c2 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -1984,7 +1984,7 @@ static int ext4_fc_replay_inode(struct super_block *sb,
>  	ret = sync_dirty_buffer(iloc.bh);
>  	if (ret)
>  		goto out_brelse;
> -	ret = ext4_mark_inode_used(sb, ino);
> +	ret = ext4_mark_inode_used(sb, ino, le16_to_cpu(raw_fc_inode->i_mode));
>  	if (ret)
>  		goto out_brelse;
>  

[Severity: Medium]
Can dereferencing raw_fc_inode->i_mode trigger an unaligned memory access
fault on strict-alignment architectures?

Fast commit TLVs are packed contiguously without alignment padding, so an
earlier dentry tag with an odd-length filename places subsequent
EXT4_FC_TAG_INODE payloads and raw_fc_inode at an odd byte offset.
ext4_fc_replay_inode() already copies raw_fc_inode into the aligned
raw_inode buffer earlier in the function, so reading raw_inode->i_mode
instead avoids the unaligned load.

> @@ -2041,11 +2041,6 @@ static int ext4_fc_replay_create(struct super_block *sb,
>  	trace_ext4_fc_replay(sb, EXT4_FC_TAG_CREAT, darg.ino,
>  			darg.parent_ino, darg.dname_len);
>  
> -	/* This takes care of update group descriptor and other metadata */
> -	ret = ext4_mark_inode_used(sb, darg.ino);
> -	if (ret)
> -		goto out;
> -
>  	inode = ext4_iget(sb, darg.ino, EXT4_IGET_NORMAL);
>  	if (IS_ERR(inode)) {
>  		ext4_debug("inode %d not found.", darg.ino);
> @@ -2054,6 +2049,11 @@ static int ext4_fc_replay_create(struct super_block *sb,
>  		goto out;
>  	}
>  
> +	/* This takes care of update group descriptor and other metadata */
> +	ret = ext4_mark_inode_used(sb, darg.ino, inode->i_mode);
> +	if (ret)
> +		goto out;
> +
>  	if (S_ISDIR(inode->i_mode)) {
>  		/*
>  		 * If we are creating a directory, we need to make sure that the

[Severity: Critical]
This problem wasn't introduced by this patch, but does
ext4_fc_replay_link_internal() [1] silently return 0 when looking up
the parent directory fails?

fs/ext4/fast_commit.c:ext4_fc_replay_link_internal() {
	...
	int ret = 0;

	dir = ext4_iget(sb, darg->parent_ino, EXT4_IGET_NORMAL);
	if (IS_ERR(dir)) {
		ext4_debug("Dir with inode %d not found.", darg->parent_ino);
		dir = NULL;
		goto out;
	}
	...
out:
	if (dir)
		iput(dir);

	return ret;
}

When ext4_fc_replay_create() calls ext4_fc_replay_link_internal() and the
parent lookup fails, it receives 0 and proceeds to set i_nlink to 1 and
mark the inode dirty, leaving an unlinked inode and inconsistent metadata
instead of aborting recovery.

[1] https://sashiko.dev/#/bug/linux-f99925c6-c893-41e7-947b-8df2ac40e92c

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