Re: [PATCH -RFC] ext4: add KUinit tests for fs/ext4/extents_status.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ext4: use-after-free in es_cleanup_inode() due to KUnit teardown order
- [Medium] ext4: lock leak and sleep-in-atomic in assert_es_tree() under i_es_lock
- [Medium] ext4: extents_status-test: unlock sb->s_umount before kunit_activate_static_stub()
--

commit 68a97fa816f24b4b23fae3c4dae5fc1c10033a0d
Author: Theodore Tso <tytso@mit.edu>

ext4: add KUinit tests for fs/ext4/extents_status.c

This commit adds KUnit tests for the ext4 extents status tree and bigalloc
pending reservations, along with static stub redirects and symbol exports
needed by the test module.

> diff --git a/fs/ext4/extents_status-test.c b/fs/ext4/extents_status-test.c
> new file mode 100644
> index 0000000000000..65376666a362f
> --- /dev/null
> +++ b/fs/ext4/extents_status-test.c
> @@ -0,0 +1,1509 @@

[ ... ]

> +static void es_cleanup_inode(struct inode *inode)
> +{
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> +	struct rb_node *node;
> +
> +	sbi->s_mount_state &= ~EXT4_FC_REPLAY;
> +	ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
> +
> +	while ((node = rb_first(&ei->i_pending_tree.root)) != NULL) {
> +		struct pending_reservation *pr;
> +
> +		pr = rb_entry(node, struct pending_reservation, rb_node);
> +		ext4_remove_pending(inode, EXT4_C2B(sbi, pr->lclu));
> +	}
> +}
> +
> +KUNIT_DEFINE_ACTION_WRAPPER(es_cleanup_inode_action, es_cleanup_inode,
> +			    struct inode *);
> +
> +static struct inode *es_alloc_test_inode(struct kunit *test,
> +					 struct super_block *sb,
> +					 unsigned long ino)
> +{
> +	struct ext4_inode_info *ei;
> +	int ret;
> +
> +	ei = kunit_kzalloc(test, sizeof(*ei), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, ei);
> +	es_init_mock_inode(ei, sb, ino);
> +
> +	ret = kunit_add_action_or_reset(test, es_cleanup_inode_action,
> +					&ei->vfs_inode);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	return &ei->vfs_inode;
> +}

[ ... ]

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

[ ... ]

> +	es_init_mock_inode(ctx->ei, sb, 12);
> +	ctx->sb = sb;
> +	ctx->sbi = sbi;
> +	ctx->shrinker_registered = true;
> +	test->priv = ctx;
> +
> +	kunit_activate_static_stub(test, ext4_da_update_reserve_space,
> +				   ext4_da_update_reserve_space_stub);
> +	kunit_activate_static_stub(test, ext4_da_release_space,
> +				   ext4_da_release_space_stub);
> +
> +	up_write(&sb->s_umount);
> +	return 0;
> +}

[Severity: Medium]
Should up_write(&sb->s_umount) happen before kunit_activate_static_stub()?

sget_fc() returns with sb->s_umount held write-locked, and test->priv = ctx is
assigned before calling kunit_activate_static_stub(). Inside
__kunit_activate_static_stub(), an allocation failure triggers
KUNIT_ASSERT_NOT_ERR_OR_NULL(), which aborts the init kthread via
kunit_try_catch_throw() before up_write(&sb->s_umount) is reached.

When KUnit then runs es_kunit_exit() in the cleanup kthread,
deactivate_super(ctx->sb) -> __super_lock_excl() will attempt to acquire
sb->s_umount and deadlock.

> +
> +static void es_kunit_exit(struct kunit *test)
> +{
> +	struct es_test_ctx *ctx = test->priv;
> +
> +	if (!ctx)
> +		return;
> +
> +	if (ctx->ei && ctx->sb)
> +		es_cleanup_inode(&ctx->ei->vfs_inode);
> +	if (ctx->shrinker_registered)
> +		ext4_es_unregister_shrinker(ctx->sbi);
> +	if (ctx->sb)
> +		deactivate_super(ctx->sb);
> +}

[Severity: High]
Can registering es_cleanup_inode_action() via kunit_add_action_or_reset() in
es_alloc_test_inode() cause a use-after-free when KUnit tears down the test?

In lib/kunit/test.c, kunit_run_case_cleanup() runs suite->exit(test) before
unwinding test resources via kunit_case_internal_cleanup():

kunit_run_case_cleanup()
  suite->exit(test) -> es_kunit_exit()
    ext4_es_unregister_shrinker(ctx->sbi)
    deactivate_super(ctx->sb)
  kunit_case_internal_cleanup(test)
    kunit_cleanup(test)
      es_cleanup_inode_action(inode2)
        es_cleanup_inode(inode2)

When test_es_shrinker_skips_precached_inode_until_last_resort() allocates
inode2 with es_alloc_test_inode(), es_kunit_exit() destroys the s_es_stats
per-CPU counters via ext4_es_unregister_shrinker() and drops the superblock
reference via deactivate_super(ctx->sb) before es_cleanup_inode(inode2) runs.

es_cleanup_inode(inode2) and ext4_es_remove_extent() then dereference
inode->i_sb after deactivate_super(ctx->sb). If inode2 still has any extents
in i_es_tree (for example if an earlier assertion fails before inode2 is
shrunk), ext4_es_remove_extent() -> ext4_es_free_extent() will also call
percpu_counter_dec() on the already-destroyed s_es_stats counters where
fbc->counters is NULL.

> +
> +static void assert_es_tree(struct kunit *test, struct inode *inode,
> +			   const struct expected_es *exp, unsigned int nr_exp)
> +{
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	struct rb_node *node;
> +	unsigned int i = 0;
> +	ext4_lblk_t prev_end = 0;
> +
> +	read_lock(&ei->i_es_lock);
> +	for (node = rb_first(&ei->i_es_tree.root); node; node = rb_next(node)) {
> +		struct extent_status *es =
> +			rb_entry(node, struct extent_status, rb_node);
> +
> +		KUNIT_ASSERT_LT_MSG(test, i, nr_exp,
> +				    "unexpected extra extent [%u/%u) type 0x%x",
> +				    es->es_lblk, es->es_len, ext4_es_type(es));
> +
> +		if (i > 0)
> +			KUNIT_EXPECT_GT_MSG(test, es->es_lblk, prev_end,
> +					    "extent %u not strictly ordered",
> +					    i);
> +
> +		KUNIT_EXPECT_EQ_MSG(test, es->es_lblk, exp[i].lblk,
> +				    "extent %u lblk mismatch", i);
> +		KUNIT_EXPECT_EQ_MSG(test, es->es_len, exp[i].len,
> +				    "extent %u len mismatch", i);
> +		KUNIT_EXPECT_EQ_MSG(test, ext4_es_type(es), exp[i].type,
> +				    "extent %u type mismatch", i);
> +
> +		if (exp[i].type & (EXTENT_STATUS_WRITTEN |
> +				   EXTENT_STATUS_UNWRITTEN))
> +			KUNIT_EXPECT_EQ_MSG(test, ext4_es_pblock(es),
> +					    exp[i].pblk,
> +					    "extent %u pblk mismatch", i);
> +
> +		prev_end = es->es_lblk + es->es_len - 1;
> +		i++;
> +	}
> +	read_unlock(&ei->i_es_lock);
> +
> +	KUNIT_EXPECT_EQ(test, i, nr_exp);
> +	KUNIT_EXPECT_EQ(test, ei->i_es_all_nr, nr_exp);
> +}

[Severity: Medium]
Is it safe to invoke KUNIT_ASSERT_LT_MSG() and KUNIT_EXPECT_*_MSG() while
holding read_lock(&ei->i_es_lock)?

When a KUnit expectation or assertion fails, __kunit_do_failed_assertion() ->
kunit_fail() calls kunit_alloc_string_stream(test, GFP_KERNEL), which performs
a sleeping allocation under the ei->i_es_lock rwlock.

In addition, if KUNIT_ASSERT_LT_MSG() fails, __kunit_abort() exits the test
kthread immediately without calling read_unlock(&ei->i_es_lock). During
teardown, es_kunit_exit() -> es_cleanup_inode() -> ext4_es_remove_extent()
will then call write_lock(&EXT4_I(inode)->i_es_lock) and deadlock on the
leaked read lock.

[ ... ]

> +static void
> +test_es_shrinker_skips_precached_inode_until_last_resort(struct kunit *test)
> +{
> +	struct es_test_ctx *ctx = test->priv;
> +	struct inode *inode1 = &ctx->ei->vfs_inode;
> +	struct inode *inode2;
> +	struct shrinker *shrink = ctx->sbi->s_es_shrinker;
> +	struct shrink_control sc;
> +	unsigned long shrunk;
> +
> +	inode2 = es_alloc_test_inode(test, ctx->sb, 13);

[ ... ]

> +}

[ ... ]

> -- 
> 2.51.2
>

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