Re: [PATCH 09/27] ext4: add KUnit tests for fs/ext4/fast_commit.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] ext4: uninitialized j_trans_commit_map in
  fast_commit_test_init()
- [Medium] ext4: excessive stack frame size in fast_commit-test.c test
  cases
- [Low] ext4: resource leak and UAF on abort in fast_commit_test_init()
  and cleanup
--

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

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

This commit adds the ext4_fast_commit_test KUnit test suite in
fs/ext4/fast_commit-test.c and exports fast commit helpers and static stub
hooks in fs/ext4/fast_commit.c and related files.

> diff --git a/fs/ext4/fast_commit-test.c b/fs/ext4/fast_commit-test.c
> new file mode 100644
> index 0000000000000..57745e51bea67
> --- /dev/null
> +++ b/fs/ext4/fast_commit-test.c
[ ... ]
> +/* --- Suite Setup & Teardown --- */
> +
> +static void fc_cleanup_all(void *priv)
> +{
> +	struct fc_test_ctx *ctx = priv;
> +	int i;
> +
> +	ctx->sbi->s_mount_state &= ~EXT4_FC_REPLAY;
> +	ext4_fc_cleanup_test(&ctx->journal, 1, 999999);
> +	ext4_fc_replay_cleanup(ctx->sb);
> +
> +	for (i = 0; i < TEST_MAX_INODES; i++) {
> +		if (!ctx->inodes[i].allocated)
> +			continue;
> +		down_write(&ctx->inodes[i].ei.i_data_sem);
> +		ext4_es_remove_extent(&ctx->inodes[i].ei.vfs_inode, 0,
> +				      EXT_MAX_BLOCKS);
> +		up_write(&ctx->inodes[i].ei.i_data_sem);
> +		ext4_fc_free_inode_snap_test(&ctx->inodes[i].ei.vfs_inode);
> +	}
> +
> +	ext4_es_unregister_shrinker(ctx->sbi);
> +	deactivate_super(ctx->sb);
> +}
> +
> +static int fast_commit_test_init(struct kunit *test)
> +{
> +	struct fc_test_ctx *ctx;
> +	struct ext4_sb_info *sbi;
> +	struct super_block *sb;
> +	struct fs_context *fc;
> +	int i, err;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +	sbi = kunit_kzalloc(test, sizeof(*sbi), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sbi);
> +
> +	fc = fs_context_for_mount(&fc_fs_type, 0);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR(fc));
> +	sb = sget_fc(fc, NULL, fc_set_anon);
> +	put_fs_context(fc);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR(sb));
[ ... ]
> +	up_write(&sb->s_umount);
> +
> +	err = ext4_es_register_shrinker(sbi);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +	ctx->shrinker_count = sbi->s_es_shrinker->count_objects;
> +	ctx->shrinker_scan = sbi->s_es_shrinker->scan_objects;
> +	WRITE_ONCE(sbi->s_es_shrinker->count_objects, fc_dummy_shrinker_count);
> +	WRITE_ONCE(sbi->s_es_shrinker->scan_objects, fc_dummy_shrinker_scan);
> +
> +	/* Mock journal, running transaction, and handle setup */
> +	rwlock_init(&ctx->journal.j_state_lock);
> +	spin_lock_init(&ctx->journal.j_list_lock);
> +	mutex_init(&ctx->journal.j_barrier);

[Severity: Medium]
Does fast_commit_test_init() leave ctx->journal.j_trans_commit_map
uninitialized?

ctx is heap-allocated with kunit_kzalloc().  When test_fc_commit_paths()
calls ext4_fc_commit() -> ext4_fc_perform_commit() ->
jbd2_journal_lock_updates(), jbd2_might_wait_for_commit() passes
&ctx->journal.j_trans_commit_map to rwsem_acquire() and rwsem_release().
With CONFIG_LOCKDEP enabled, its NULL key in non-static memory triggers
"INFO: trying to register non-static key" and turns off lockdep via
debug_locks_off().

[ ... ]

> +	kunit_activate_static_stub(test, ext4_fc_submit_bh,
> +				   stub_fc_submit_bh);
> +	kunit_activate_static_stub(test, ext4_fc_issue_flush,
> +				   stub_fc_issue_flush);
> +
> +	init_mock_inode_slot(ctx, &ctx->inodes[0], TEST_ROOT_INO,
> +			     S_IFDIR | 0755);
> +	init_mock_inode_slot(ctx, &ctx->inodes[1], TEST_DIR_INO,
> +			     S_IFDIR | 0755);
> +	init_mock_inode_slot(ctx, &ctx->inodes[2], TEST_FILE1_INO,
> +			     S_IFREG | 0644);
> +	init_mock_inode_slot(ctx, &ctx->inodes[3], TEST_FILE2_INO,
> +			     S_IFREG | 0644);
> +	init_mock_inode_slot(ctx, &ctx->inodes[4], TEST_SUBDIR_INO,
> +			     S_IFDIR | 0755);
> +
> +	err = kunit_add_action_or_reset(test, fc_cleanup_all, ctx);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	return 0;
> +}

[Severity: Low]
Can an assertion failure in fast_commit_test_init() leak sb and leave
sbi->s_es_shrinker and sbi->s_es_stats percpu_counters registered after
KUnit frees sbi?

fc_cleanup_all() is only registered after ext4_es_register_shrinker() and
all the kunit_activate_static_stub() calls, which themselves assert on
allocation failure.  Aborting in that window skips deactivate_super(sb) and
ext4_es_unregister_shrinker(sbi), causing a use-after-free once the
kunit_kzalloc()-allocated sbi is freed.

Also, in fc_cleanup_all(), calling ext4_fc_cleanup_test() only once drains
FC_Q_MAIN before splicing FC_Q_STAGING into FC_Q_MAIN, so any
ext4_fc_dentry_update entries on s_fc_dentry_q[FC_Q_STAGING] when a test
aborts are leaked.

> +
> +/* =========================================================================
> + * 1. Initialization, Eligibility, and Snapshot Stat Helpers
> + * =========================================================================
> + */
> +
> +static void test_fc_init_and_eligibility(struct kunit *test)
> +{
> +	struct fc_test_ctx *ctx = test->priv;
> +	struct super_block *sb = ctx->sb;
> +	struct inode *inode = get_mock_inode(ctx, TEST_FILE1_INO);
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	journal_t tmp_journal = {};

[Severity: Medium]
Does allocating tmp_journal on the stack here (and five struct dentry
objects on the stack in test_fc_replay_inode_and_dentries()) exceed
CONFIG_FRAME_WARN?

journal_t is over 1.3 KB (and over 2.2 KB with CONFIG_LOCKDEP enabled), and
the five dentries in test_fc_replay_inode_and_dentries() take ~1.6 KB with
CONFIG_LOCKDEP, triggering -Wframe-larger-than warnings or build failures
under CONFIG_WERROR.  Could tmp_journal and the mock dentries be allocated
with kunit_kzalloc() or moved into struct fc_test_ctx?

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