Re: [PATCH 18/27] ext4: add KUnit tests for fs/ext4/sysfs.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] ext4: UAF of ctx->sb in fserror_worker() due to NULL
  sb->s_root in sysfs-test.c
- [Low] ext4: preemptible part_stat_add() call in
  test_computed_ro_and_error_time_attrs()
- [Low] ext4: s_last_error_func check fails under !CONFIG_PRINTK in
  sysfs-test.c
- [Low] ext4: sysfs_test_cleanup() frees ctx->sb before canceling work
  and timer
--

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

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

This commit adds a KUnit test suite covering ext4 sysfs attribute show and
store handlers, feature capability attributes, and sysfs/procfs registration.

> diff --git a/fs/ext4/sysfs-test.c b/fs/ext4/sysfs-test.c
> new file mode 100644
> index 0000000000000..c13785420aa11
> --- /dev/null
> +++ b/fs/ext4/sysfs-test.c
> @@ -0,0 +1,653 @@

[ ... ]

> +static void sysfs_test_cleanup(struct sysfs_test_ctx *ctx)
> +{
> +	ctx->sbi.s_journal = NULL;
> +
> +	deactivate_super(ctx->sb);
> +	rcu_barrier();
> +
> +	cancel_work_sync(&ctx->sbi.s_sb_upd_work);
> +	timer_delete_sync(&ctx->sbi.s_err_report);

[Severity: Low]
Should cancel_work_sync() and timer_delete_sync() run before
deactivate_super(ctx->sb)?

Writing a non-zero value to err_report_sec in test_validated_rw_attrs()
replaces sbi->s_err_report's callback with print_daily_error_info() and arms
the timer, and print_daily_error_info() dereferences sbi->s_sb (ctx->sb).
Stopping the work item and timer before deactivating and RCU-freeing ctx->sb
ensures neither callback can access ctx->sb after it is freed.

[ ... ]

> +	sbi->s_sb = sb;
> +	sb->s_fs_info = sbi;
> +	sb->s_op = &sysfs_test_sops;
> +#ifdef CONFIG_FS_ENCRYPTION
> +	sb->s_cop = &ext4_cryptops;
> +#endif
> +	sb->s_flags |= SB_ACTIVE;
> +	sb->s_blocksize = SYSFS_TEST_BLK_SIZE;
> +	sb->s_blocksize_bits = SYSFS_TEST_BLK_BITS;
> +	strscpy(sb->s_id, "ext4_sysfs_kunit", sizeof(sb->s_id));

[Severity: Medium]
Can leaving sb->s_root NULL while setting SB_ACTIVE cause a use-after-free of
ctx->sb in fserror_worker()?

In test_feature_attrs_and_trigger_error(), writing to trigger_fs_error calls
trigger_test_error() -> ext4_error() -> __ext4_error() ->
fserror_report_metadata().  Because alloc_super() initialized
sb->s_pending_errors to 1 and SB_ACTIVE is set, fserror_report_metadata()
queues an asynchronous fserror_worker work item holding a raw pointer to
ctx->sb.  However, generic_shutdown_super() only clears SB_ACTIVE and calls
fserror_unmount(sb) to drain pending events when sb->s_root is non-NULL:

CPU0                                      kworker
test_feature_attrs_and_trigger_error()
  ...
    fserror_report_metadata(sb, ...)
      schedule_work(&event->work)
sysfs_test_cleanup()
  deactivate_super(ctx->sb)
    kill_anon_super()
      generic_shutdown_super()
        /* sb->s_root is NULL:
         * skips fserror_unmount(sb)
         */
      put_super(sb)
  rcu_barrier()
  /* destroy_super_work() frees sb */
                                          fserror_worker()
                                            sb = event->sb;
                                            if (!(sb->s_flags & SB_ACTIVE))
                                              <-- use-after-free

Initializing sb->s_root with d_make_root() (or calling fserror_unmount(ctx->sb)
before deactivating the superblock) would ensure pending fserror events are
drained before ctx->sb is freed.

[ ... ]

> +static void test_computed_ro_and_error_time_attrs(struct kunit *test)
> +{
> +	struct sysfs_test_ctx *ctx = test->priv;
> +	struct ext4_sb_info *sbi = &ctx->sbi;
> +	struct ext4_super_block *es = &ctx->es;
> +	char expected_pid[32];
> +	char *buf;
> +
> +	buf = kunit_kzalloc(test, PAGE_SIZE, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, buf);
> +
> +	/* 7 dirty clusters * 4 blocks/cluster == 28 delayed allocation blks */
> +	percpu_counter_set(&sbi->s_dirtyclusters_counter, 7);
> +	KUNIT_EXPECT_GT(test,
> +			ext4_sysfs_attr_show_test(sbi,
> +				"delayed_allocation_blocks", buf),
> +			(ssize_t)0);
> +	KUNIT_EXPECT_STREQ(test, buf, "28\n");
> +
> +	/* (50 - 10) sectors >> 1 == 20 KB session, + 100 == 120 KB lifetime */
> +	sbi->s_sectors_written_start = 10;
> +	sbi->s_kbytes_written = 100;
> +	part_stat_add(&ctx->bdev, sectors[STAT_WRITE], 50);

[Severity: Low]
Does calling part_stat_add() here without part_stat_lock() /
part_stat_unlock() trigger a CONFIG_DEBUG_PREEMPT warning?

part_stat_add() uses __this_cpu_add(), which requires preemption to be
disabled and logs a "BUG: using __this_cpu_add() in preemptible" warning when
called in a preemptible KUnit thread with CONFIG_DEBUG_PREEMPT enabled.

[ ... ]

> +	/* Non-empty message records error state via ext4_error() */
> +	KUNIT_EXPECT_EQ(test,
> +			ext4_sysfs_attr_store_test(sbi, "trigger_fs_error",
> +						   "kunit injected\n", 15),
> +			(ssize_t)15);
> +	KUNIT_EXPECT_NE(test, sbi->s_mount_state & EXT4_ERROR_FS, 0);
> +	KUNIT_EXPECT_STREQ(test, sbi->s_last_error_func, "trigger_test_error");
> +	sbi->s_journal = NULL;
> +}

[Severity: Low]
Will the sbi->s_last_error_func assertion fail when CONFIG_PRINTK is disabled?

In fs/ext4/ext4.h, the !CONFIG_PRINTK definition of ext4_error() passes ""
instead of __func__ to __ext4_error(), so sbi->s_last_error_func is set to ""
rather than "trigger_test_error".

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