Re: [PATCH 22/27] ext4: add KUnit tests for fs/ext4/indirect.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ext4: indirect-test: UAF in ind_test_free_inode() after RCU
  grace period
- [Low] ext4: uncleaned percpu_counter and uninited work/timer in
  ind_test_init()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] ext4: incomplete error check in ext4_ind_map_blocks() [1]
--

commit ebeb5ffec6cf1b1539b939ab7329a1923d9ae24c
Author: Theodore Ts'o <tytso@mit.edu>

ext4: add KUnit tests for fs/ext4/indirect.c

This commit adds the ext4_indirect_test KUnit test suite for indirect block
mapping, truncation, and hole punching, and routes indirect block buffer
lookups through ext4_sb_getblk().

> diff --git a/fs/ext4/indirect-test.c b/fs/ext4/indirect-test.c
> new file mode 100644
> index 0000000000000..add5662adc3ba
> --- /dev/null
> +++ b/fs/ext4/indirect-test.c

[ ... ]

> +static void ind_test_free_inode(struct inode *inode)
> +{
> +	ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
> +	kfree(EXT4_I(inode));
> +}
> +
> +static void ind_test_evict_inode(struct inode *inode)
> +{
> +	truncate_inode_pages_final(&inode->i_data);
> +	clear_inode(inode);
> +}

[Severity: Medium]
Can ind_test_free_inode() trigger a use-after-free on ctx->sbi after the
test finishes?

At the end of each test, iput() runs ind_test_evict_inode() synchronously
and queues i_callback() -> ind_test_free_inode() via call_rcu().  KUnit
then runs ind_test_cleanup() without an rcu_barrier() and frees ctx, so
when the RCU callback later runs ext4_es_remove_extent(), it dereferences
EXT4_SB(inode->i_sb) inside the freed ctx.

Moving ext4_es_remove_extent() into ind_test_evict_inode() and calling
rcu_barrier() in ind_test_cleanup() before deactivating sb would avoid
this.

[ ... ]

> +static void ind_test_cleanup(struct ind_test_ctx *ctx)
> +{
> +	struct super_block *sb = ctx->sb;
> +	struct ext4_sb_info *sbi = &ctx->sbi;
> +	struct ind_mock_blk *blk, *tmp;
> +
> +	if (sb) {
> +		cancel_work_sync(&sbi->s_sb_upd_work);
> +		timer_delete_sync(&sbi->s_err_report);
> +	}
> +
> +	if (ctx->es_shrinker_inited) {
> +		ext4_es_unregister_shrinker(sbi);
> +		ctx->es_shrinker_inited = false;
> +	}
> +
> +	if (ctx->counters_inited) {
> +		percpu_counter_destroy(&sbi->s_freeclusters_counter);
> +		percpu_counter_destroy(&sbi->s_dirtyclusters_counter);
> +		percpu_counter_destroy(&sbi->s_freeinodes_counter);
> +		ctx->counters_inited = false;
> +	}

[ ... ]

> +	ctx->sb = sb;
> +	up_write(&sb->s_umount);
> +
> +	ret = kunit_add_action_or_reset(test, ind_test_cleanup_action, ctx);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[ ... ]

> +	INIT_WORK(&sbi->s_sb_upd_work, ind_test_sb_upd_work);
> +	timer_setup(&sbi->s_err_report, ind_test_err_timer, 0);

[ ... ]

> +	ret = percpu_counter_init(&sbi->s_freeclusters_counter, 4096,
> +				  GFP_KERNEL);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	ret = percpu_counter_init(&sbi->s_dirtyclusters_counter, 0, GFP_KERNEL);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	ret = percpu_counter_init(&sbi->s_freeinodes_counter, 200, GFP_KERNEL);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	ctx->counters_inited = true;

[Severity: Low]
If the second or third percpu_counter_init() fails, does KUNIT_ASSERT_EQ()
abort while ctx->counters_inited is still false?  ind_test_cleanup() then
skips percpu_counter_destroy(), leaking the earlier counter and leaving it
linked in the global percpu_counters list when ctx is freed.

Also, ctx->sb is set before kunit_add_action_or_reset(), whereas
s_sb_upd_work and s_err_report are initialized later, so if
kunit_add_action_or_reset() fails, ind_test_cleanup() calls
cancel_work_sync() and timer_delete_sync() on uninitialized structures.

[ ... ]

> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index 5aec759eed705..5b2b2be73a90a 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -163,7 +163,7 @@ static Indirect *ext4_get_branch(struct inode *inode, int depth,
>  			ret = -EFSCORRUPTED;
>  			goto failure;
>  		}
> -		bh = sb_getblk(sb, key);
> +		bh = ext4_sb_getblk(sb, key);
>  		if (unlikely(!bh)) {
>  			ret = -ENOMEM;
>  			goto failure;

[Severity: Critical]
This problem wasn't introduced by this patch, but when ext4_get_branch()
fails with -EFSCORRUPTED or -ENOMEM [1], doesn't
ext4_ind_map_blocks() miss the error when EXT4_GET_BLOCKS_CREATE is set?

fs/ext4/indirect.c:ext4_ind_map_blocks() {
	...
	/* Failed read of indirect block */
	if (err == -EIO)
		goto cleanup;
	...
}

Because ext4_ind_map_blocks() only checks for err == -EIO, it treats the
partial chain returned on -ENOMEM or -EFSCORRUPTED as a hole, allocates new
blocks, and splices them over the existing indirect block pointer,
orphaning the original subtree and corrupting the file.

[1] https://sashiko.dev/#/bug/linux-2bfbc9ba-6121-45d3-a664-11e7452ad5d6

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112040638.1576836-1-tytso@mit.edu?part=22