Re: [PATCH 20/27] ext4: add KUnit tests for fs/ext4/resize.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] ext4: RCU callback context and barrier order in
  resize_test_cleanup()
- [Low] ext4: uninitialized work/timer cancel in resize_test_init()
  unwind
- [Low] ext4: leak of group_desc on assertion abort in
  resize_configure_fs()
- [Low] ext4: partial percpu_counter_init() leak in
  resize_configure_fs()
--

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

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

This commit adds a KUnit test suite for online filesystem resizing in
fs/ext4/resize.c and routes resize metadata buffer lookups and zeroout
calls through KUnit stubbed helpers.

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

[ ... ]

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

[ ... ]

> +static void resize_test_cleanup(struct resize_test_ctx *ctx)
> +{
> +	struct super_block *sb = ctx->sb;
> +	struct ext4_sb_info *sbi = &ctx->sbi;
> +	struct flex_groups **flex_groups;
> +	struct resize_mock_blk *blk, *tmp;
> +	unsigned int i;
> +
> +	if (sb) {
> +		cancel_work_sync(&sbi->s_sb_upd_work);
> +		timer_delete_sync(&sbi->s_err_report);
> +	}
> +
> +	if (ctx->resize_inode) {
> +		iput(ctx->resize_inode);
> +		ctx->resize_inode = NULL;
> +	}
> +
> +	if (ctx->mb_inited && sb) {
> +		ext4_mb_release(sb);
> +		ctx->mb_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;
> +	}
> +
> +	flex_groups = rcu_access_pointer(sbi->s_flex_groups);
> +	if (flex_groups) {
> +		for (i = 0; i < sbi->s_flex_groups_allocated; i++)
> +			kvfree(flex_groups[i]);
> +		kvfree(flex_groups);
> +		rcu_assign_pointer(sbi->s_flex_groups, NULL);
> +		sbi->s_flex_groups_allocated = 0;
> +	}
> +
> +	rcu_barrier();
> +	kvfree(rcu_access_pointer(sbi->s_group_desc));
> +	rcu_assign_pointer(sbi->s_group_desc, NULL);
> +
> +	if (sb) {
> +		deactivate_super(sb);
> +		ctx->sb = NULL;
> +	}

[Severity: Medium]
Can resize_test_free_inode() and resize_test_cleanup() cause a
use-after-free on ctx->sbi and take i_es_lock in RCU softirq context?

The VFS invokes super_operations.free_inode from i_callback() in RCU
softirq context, but resize_test_free_inode() calls
ext4_es_remove_extent(), which takes write_lock(&EXT4_I(inode)->i_es_lock)
without disabling softirqs, reads EXT4_SB(inode->i_sb)->s_mount_state, and
can touch sbi->s_dirtyclusters_counter.  In addition, resize_test_cleanup()
destroys the percpu counters before rcu_barrier() and calls
deactivate_super(sb) after rcu_barrier(); if a test aborts while an inode
such as rinode in test_convert_meta_bg_validation_and_conversion() is still
live, deactivate_super() -> evict_inodes() queues i_callback() after
rcu_barrier() has already returned, so resize_test_free_inode() can run
after KUnit frees ctx.

Moving ext4_es_remove_extent() into resize_test_evict_inode() (matching
ext4_clear_inode()) and calling deactivate_super(sb) before rcu_barrier()
and percpu_counter_destroy() would avoid both issues.

[ ... ]

> +static void resize_configure_fs(struct kunit *test, ext4_group_t ngroups,
> +				u16 reserved_gdb, u8 log_flex,
> +				u32 last_grp_blks)
> +{

[ ... ]

> +	group_desc = kvmalloc_array(gdb_count, sizeof(*group_desc), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, group_desc);
> +	for (g = 0; g < gdb_count; g++) {
> +		struct buffer_head *bh;
> +
> +		bh = resize_get_or_create_bh(ctx,
> +					     RESIZE_TEST_FIRST_DATA_BLK + 1 + g);
> +		KUNIT_ASSERT_NOT_NULL(test, bh);
> +		brelse(bh);
> +		group_desc[g] = bh;
> +	}
> +	rcu_assign_pointer(sbi->s_group_desc, group_desc);

[Severity: Low]
Does this leak group_desc if KUNIT_ASSERT_NOT_NULL(test, bh) aborts the
test?  Because sbi->s_group_desc is only assigned after the loop,
resize_test_cleanup() will not see group_desc to free it.

[ ... ]

> +	ret = percpu_counter_init(&sbi->s_freeclusters_counter,
> +				  total_free_blks, 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,
> +				  total_inodes - 10, GFP_KERNEL);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	ctx->counters_inited = true;

[Severity: Low]
What happens if the second or third percpu_counter_init() fails and
KUNIT_ASSERT_EQ() aborts before ctx->counters_inited is set?

resize_test_cleanup() skips percpu_counter_destroy() when
ctx->counters_inited is false, leaking the already-initialized counter(s)
and, under CONFIG_HOTPLUG_CPU, leaving their list_head entries inside
ctx->sbi linked on the global percpu_counters list after ctx is freed.
Since percpu_counter_destroy() is a no-op on zero-initialized counters,
resize_test_cleanup() can call percpu_counter_destroy() unconditionally.

[ ... ]

> +static int resize_test_init(struct kunit *test)
> +{

[ ... ]

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

[ ... ]

> +	INIT_WORK(&sbi->s_sb_upd_work, resize_test_sb_upd_work);
> +	timer_setup(&sbi->s_err_report, resize_test_err_timer, 0);

[Severity: Low]
If kunit_add_action_or_reset() fails to allocate the action, it immediately
invokes resize_test_cleanup() while ctx->sb is already set, before
sbi->s_sb_upd_work and sbi->s_err_report are initialized below.  Would
cancel_work_sync() and timer_delete_sync() in resize_test_cleanup() then
trigger warnings on the uninitialized work and timer structures?

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