Re: [PATCH 11/27] ext4: add KUnit tests for fs/ext4/ialloc.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ext4: UAF on super_block in ialloc_test_cleanup() from
  undrained fserror work
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] ext4: missing error handling for blkdev_issue_flush() in
  ext4_fc_commit() [1]
--

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

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

This commit adds a KUnit test suite for fs/ext4/ialloc.c along with static
stub hooks and helper exports to test inode allocation, freeing, bitmap
validation, and lazy inode table initialization.

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

[ ... ]

> +static void ialloc_test_cleanup(void *data)
> +{
> +	struct ialloc_test_ctx *ctx = data;
> +	handle_t *handle;
> +	unsigned int i;
> +
> +	ctx->evict_skip_free = true;
> +	ctx->sbi.s_journal = NULL;
> +
> +	while ((handle = ext4_journal_current_handle()) != NULL)
> +		ext4_journal_stop(handle);
> +
> +	for (i = 0; i < ctx->nr_tracked_inodes; i++) {
> +		struct inode *ti = ctx->tracked_inodes[i];
> +
> +		if (ti) {
> +			if (inode_state_read_once(ti) & I_NEW)
> +				unlock_new_inode(ti);
> +			spin_lock(&ti->i_lock);
> +			inode_state_clear(ti, I_DIRTY_TIME);
> +			spin_unlock(&ti->i_lock);
> +			set_nlink(ti, 1);
> +			iput(ti);
> +			ctx->tracked_inodes[i] = NULL;
> +		}
> +	}
> +	ctx->nr_tracked_inodes = 0;
> +
> +	ctx->sb->s_root = NULL;
> +	if (ctx->root_inode) {
> +		set_nlink(ctx->root_inode, 2);
> +		iput(ctx->root_inode);
> +		ctx->root_inode = NULL;
> +	}
> +
> +	deactivate_super(ctx->sb);

[Severity: Medium]
Can clearing ctx->sb->s_root before deactivate_super(ctx->sb) cause a
use-after-free on ctx->sb in fserror_worker()?

ialloc_test_init() sets SB_ACTIVE on ctx->sb, so tests that trigger
ext4_error() or ext4_std_error() queue fserror_worker() items referencing
ctx->sb via fserror_report_metadata().  In generic_shutdown_super(),
fserror_unmount(sb) is only called inside the if (sb->s_root) block, so
nulling s_root beforehand skips draining those pending work items before
ctx->sb is freed:

CPU0                              kworker
ialloc_test_validate_ibitmap_csum()
  ext4_error()
    fserror_report_metadata()
      queue_work(..., &event->work)
ialloc_test_cleanup()
  ctx->sb->s_root = NULL
  deactivate_super(ctx->sb)
    generic_shutdown_super()
      /* skips fserror_unmount() */
    put_super()
      ...
        kfree(sb)
                                  fserror_worker()
                                    sb = event->sb
                                    if (sb->s_flags & SB_ACTIVE)
                                      <-- use-after-free

Omitting SB_ACTIVE in ialloc_test_init() would avoid queueing fserror
events in the first place.

> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 02546b828836c..66215b6359320 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c

[ ... ]

> @@ -1521,6 +1527,23 @@ unsigned long ext4_count_dirs(struct super_block * sb)
>  	}
>  	return count;
>  }
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_count_dirs);
> +
> +int ext4_itable_zeroout(struct super_block *sb, ext4_fsblk_t blk, int num,
> +			int barrier)
> +{
> +	int ret;
> +
> +	KUNIT_STATIC_STUB_REDIRECT(ext4_itable_zeroout, sb, blk, num, barrier);
> +
> +	ret = sb_issue_zeroout(sb, blk, num, GFP_NOFS);
> +	if (ret < 0)
> +		return ret;
> +	if (barrier)
> +		blkdev_issue_flush(sb->s_bdev);
> +	return ret;
> +}
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_itable_zeroout);

[Severity: Critical]
This problem wasn't introduced by this patch, but should
ext4_itable_zeroout() check and return the error from
blkdev_issue_flush() [1]?

When barrier is set and blkdev_issue_flush() fails, ext4_itable_zeroout()
still returns 0, so ext4_init_inode_table() proceeds to set
EXT4_BG_INODE_ZEROED in the group descriptor even though the zeroed inode
table blocks may not have been flushed to stable storage before a crash.
(The same unchecked blkdev_issue_flush() pattern also exists in
ext4_fc_perform_commit() and ext4_fc_replay_inode() in
fs/ext4/fast_commit.c.)

[1] https://sashiko.dev/#/bug/linux-d85ff7fd-2937-4bb0-8016-be8fb0499fbd

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