Re: [PATCH 24/27] ext4: add KUnit tests for fs/ext4/fsmap.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] ext4: per-CPU disk_stats leak on assert abort in
  fsmap_create_ctx()
- [Low] ext4: unused fail_err field in fsmap_rec_sink and
  fsmap_collect_cb()
--

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

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

This commit adds a KUnit test suite covering FS_IOC_GETFSMAP reverse-mapping
queries in fs/ext4/fsmap.c.

> diff --git a/fs/ext4/fsmap-test.c b/fs/ext4/fsmap-test.c
> new file mode 100644
> index 0000000000000..63a1061b413a7
> --- /dev/null
> +++ b/fs/ext4/fsmap-test.c
> @@ -0,0 +1,787 @@

[ ... ]

> +struct fsmap_rec_sink {
> +	struct ext4_fsmap recs[FSMAP_TEST_MAX_RECS];
> +	unsigned int count;
> +	unsigned int abort_after;
> +	int fail_err;
> +};

[ ... ]

> +static int fsmap_collect_cb(struct ext4_fsmap *fmr, void *priv)
> +{
> +	struct fsmap_rec_sink *sink = priv;
> +
> +	if (sink->fail_err)
> +		return sink->fail_err;

[Severity: Low]
Nothing in this file sets sink->fail_err, leaving the field and this check
in fsmap_collect_cb() unused.

[ ... ]

> +static struct fsmap_test_ctx *fsmap_create_ctx(struct kunit *test,
> +					       unsigned int blocksize_bits,
> +					       unsigned int cluster_bits,
> +					       ext4_group_t groups_count,
> +					       ext4_grpblk_t blocks_per_group)
> +{
> +	struct fsmap_test_ctx *ctx;
> +	struct fs_context *fc;
> +	struct super_block *sb;
> +	struct ext4_sb_info *sbi;
> +	unsigned int blocksize = 1U << blocksize_bits;
> +	ext4_fsblk_t bofs = (blocksize == 1024) ? 1 : 0;
> +	int err;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> +
> +	inode_init_once(&ctx->data_bdev.vfs_inode);
> +	address_space_init_once(&ctx->data_bdev.vfs_inode.i_data);
> +	ctx->data_bdev.vfs_inode.i_data.host = &ctx->data_bdev.vfs_inode;
> +	ctx->data_bdev.bdev.bd_dev = FSMAP_TEST_DATADEV;
> +	ctx->data_bdev.bdev.bd_mapping = &ctx->data_bdev.vfs_inode.i_data;
> +	ctx->data_bdev.bdev.bd_stats = alloc_percpu(struct disk_stats);
> +	KUNIT_ASSERT_NOT_NULL(test, ctx->data_bdev.bdev.bd_stats);
> +
> +	inode_init_once(&ctx->log_bdev.vfs_inode);
> +	address_space_init_once(&ctx->log_bdev.vfs_inode.i_data);
> +	ctx->log_bdev.vfs_inode.i_data.host = &ctx->log_bdev.vfs_inode;
> +	ctx->log_bdev.bdev.bd_dev = FSMAP_TEST_LOGDEV;
> +	ctx->log_bdev.bdev.bd_mapping = &ctx->log_bdev.vfs_inode.i_data;
> +	ctx->log_bdev.bdev.bd_stats = alloc_percpu(struct disk_stats);
> +	KUNIT_ASSERT_NOT_NULL(test, ctx->log_bdev.bdev.bd_stats);
> +	ctx->log_bdev_file.f_mapping = &ctx->log_bdev.vfs_inode.i_data;
> +
> +	sbi = &ctx->sbi;
> +	INIT_WORK(&sbi->s_sb_upd_work, fsmap_noop_work);
> +	timer_setup(&sbi->s_err_report, fsmap_noop_timer, 0);
> +	spin_lock_init(&sbi->s_error_lock);
> +	spin_lock_init(&sbi->s_bdev_wb_lock);
> +	ratelimit_state_init(&sbi->s_err_ratelimit_state,
> +			     DEFAULT_RATELIMIT_INTERVAL,
> +			     DEFAULT_RATELIMIT_BURST);
> +
> +	fc = fs_context_for_mount(&fsmap_test_fs_type, 0);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR(fc));
> +	sb = sget_fc(fc, NULL, fsmap_test_set_super);
> +	put_fs_context(fc);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR(sb));
> +	up_write(&sb->s_umount);
> +
> +	ctx->sb = sb;
> +	err = kunit_add_action_or_reset(test, fsmap_cleanup_ctx, ctx);
> +	KUNIT_ASSERT_EQ(test, err, 0);

[Severity: Low]
Does this leak the per-CPU bd_stats allocations if one of the assertions
before kunit_add_action_or_reset() aborts the test?  Registering
fsmap_cleanup_ctx() before calling alloc_percpu() would ensure they are
freed on early assertion failure.

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