Re: [PATCH 19/27] ext4: add KUnit tests for fs/ext4/ioctl.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] ext4: uninitialized s_es_stats counters dereferenced in
  ioctl_test_init()
- [Low] ext4: uninitialized work and timer cleanup in ioctl_test_init()
  error paths
- [Low] ext4: NULL bh->b_bdev in ioctl_init_mock_bh() dereferenced by
  tracepoint
- [Low] ext4: leaked inodes and dentries on KUNIT_ASSERT_NOT_NULL()
  abort in ioctl-test
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [High] ext4: missing s_encoding initialization in
  ext4_ioctl_set_tune_sb() [1]
- [High] ext4: torn read of i_version in swap_inode_data() [2]
--

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

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

This commit adds a KUnit test suite for superblock online tuning, backup
superblock updates, inode data swapping, fileattr handling, forced shutdown,
and ioctl dispatch in fs/ext4/ioctl.c, exporting helper wrappers for testing.

> diff --git a/fs/ext4/ioctl-test.c b/fs/ext4/ioctl-test.c
> new file mode 100644
> index 0000000000000..fa31a9cc5d920
> --- /dev/null
> +++ b/fs/ext4/ioctl-test.c
[ ... ]
> +static void ioctl_init_mock_bh(struct buffer_head *bh, struct folio *folio,
> +			       u8 *data, size_t size, sector_t blocknr)
> +{
> +	memset(bh, 0, sizeof(*bh));
> +	bh->b_folio = folio;
> +	bh->b_data = (char *)data;
> +	bh->b_size = size;
> +	bh->b_blocknr = blocknr;
> +	atomic_set(&bh->b_count, 2);
> +	set_buffer_uptodate(bh);
> +}

[Severity: Low]
Can leaving bh->b_bdev NULL here cause a NULL pointer dereference when the
block_dirty_buffer tracepoint is enabled?

test_update_superblocks_and_overhead() marks these mock buffer_heads dirty
via ext4_update_primary_sb() and ext4_update_backup_sb() ->
mark_buffer_dirty(), which calls trace_block_dirty_buffer(bh) and
dereferences bh->b_bdev->bd_dev.

[ ... ]
> +static void ioctl_test_cleanup(struct ioctl_test_ctx *ctx)
> +{
> +	active_ioctl_test = NULL;
> +
> +	if (ctx->sb) {
> +		deactivate_super(ctx->sb);
> +		rcu_barrier();
> +	}
> +
> +	cancel_work_sync(&ctx->sbi.s_sb_upd_work);
> +	timer_delete_sync(&ctx->sbi.s_err_report);
[ ... ]
> +static int ioctl_test_init(struct kunit *test)
> +{
[ ... ]
> +	ctx = kzalloc_obj(*ctx);
> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> +	test->priv = ctx;
> +	active_ioctl_test = test;
[ ... ]
> +	err = percpu_counter_init(&sbi->s_freeclusters_counter, 0, GFP_KERNEL);
> +	if (err) {
> +		up_write(&sb->s_umount);
> +		ioctl_test_cleanup(ctx);
> +		KUNIT_ASSERT_EQ(test, err, 0);
> +	}
> +	err = percpu_counter_init(&sbi->s_dirtyclusters_counter, 0, GFP_KERNEL);
> +	if (err) {
> +		percpu_counter_destroy(&sbi->s_freeclusters_counter);
> +		up_write(&sb->s_umount);
> +		ioctl_test_cleanup(ctx);
> +		KUNIT_ASSERT_EQ(test, err, 0);
> +	}
> +	err = percpu_counter_init(&sbi->s_freeinodes_counter, 0, GFP_KERNEL);
> +	if (err) {
> +		percpu_counter_destroy(&sbi->s_dirtyclusters_counter);
> +		percpu_counter_destroy(&sbi->s_freeclusters_counter);
> +		up_write(&sb->s_umount);
> +		ioctl_test_cleanup(ctx);
> +		KUNIT_ASSERT_EQ(test, err, 0);
> +	}
> +	ctx->counters_inited = true;
> +
> +	INIT_WORK(&sbi->s_sb_upd_work, ioctl_test_sb_upd_work);
> +	timer_setup(&sbi->s_err_report, ioctl_test_err_timer, 0);

[Severity: Low]
If any of the percpu_counter_init() calls fail, does ioctl_test_cleanup() run
cancel_work_sync() and timer_delete_sync() on uninitialized s_sb_upd_work and
s_err_report?

Because INIT_WORK() and timer_setup() happen after these error branches,
cancel_work_sync() hits WARN_ON(!work->func) in __flush_work() and
timer_delete_sync() passes an uninitialized lockdep_map to
lock_map_acquire().  Also, the earlier allocation failure paths before
sget_fc() abort without resetting active_ioctl_test to NULL.

> +	spin_lock_init(&sbi->s_error_lock);
> +	spin_lock_init(&sbi->s_md_lock);
> +	spin_lock_init(&sbi->s_bdev_wb_lock);
> +	spin_lock_init(&sbi->s_es_lock);
> +	INIT_LIST_HEAD(&sbi->s_es_list);

[Severity: Medium]
Does ioctl_test_init() leave the sbi->s_es_stats per-CPU counters
uninitialized?

test_swap_inode_data_and_seed() and test_force_shutdown_and_ioctl_dispatch()
insert and remove extents via ext4_es_insert_extent() and
ext4_es_remove_extent(), which call percpu_counter_inc() and
percpu_counter_dec() on sbi->s_es_stats.es_stats_shk_cnt and
sbi->s_es_stats.es_stats_all_cnt.  Because neither percpu_counter_init() nor
ext4_es_register_shrinker() is called for sbi, running these tests on
CONFIG_SMP dereferences a NULL per-CPU pointer and uses an uninitialized
spinlock in percpu_counter_add_batch().

[ ... ]
> +static void test_swap_inode_data_and_seed(struct kunit *test)
> +{
[ ... ]
> +	inode1 = new_inode(sb);
> +	inode2 = new_inode(sb);
> +	KUNIT_ASSERT_NOT_NULL(test, inode1);
> +	KUNIT_ASSERT_NOT_NULL(test, inode2);

[Severity: Low]
If the second new_inode(sb) call returns NULL, does KUNIT_ASSERT_NOT_NULL()
abort the test while leaking inode1?

When ioctl_test_cleanup() subsequently calls deactivate_super(ctx->sb), the
unreleased inode remains on sb->s_inodes and triggers the busy-inodes
CHECK_DATA_CORRUPTION() in generic_shutdown_super().  The same pattern
appears in test_immutable_dax_and_fileattr() when allocating reg_inode,
dir_inode, reg_dentry, and dir_dentry before d_instantiate(), and in
test_force_shutdown_and_ioctl_dispatch() if d_alloc_anon() fails after
new_inode().

> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index c8387e6a2c6e9..61e59c30e7611 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
[ ... ]
> @@ -1465,34 +1468,35 @@ static int ext4_ioctl_set_tune_sb(struct file *filp,
>  		 * Filter out the features that are already set from
>  		 * the set_mask.
>  		 */
> -		params.set_feature_compat_mask &=
> +		params->set_feature_compat_mask &=
>  			~le32_to_cpu(es->s_feature_compat);
> -		params.set_feature_incompat_mask &=
> +		params->set_feature_incompat_mask &=
>  			~le32_to_cpu(es->s_feature_incompat);
> -		params.set_feature_ro_compat_mask &=
> +		params->set_feature_ro_compat_mask &=
>  			~le32_to_cpu(es->s_feature_ro_compat);
> -		if ((params.set_feature_incompat_mask &
> -		     EXT4_FEATURE_INCOMPAT_CASEFOLD)) {
> +		if (params->set_feature_incompat_mask &
> +		    EXT4_FEATURE_INCOMPAT_CASEFOLD) {
>  			enabling_casefold = 1;
> -			if (!(params.set_flags & EXT4_TUNE_FL_ENCODING)) {
> -				params.encoding = EXT4_ENC_UTF8_12_1;
> -				params.set_flags |= EXT4_TUNE_FL_ENCODING;
> +			if (!(params->set_flags & EXT4_TUNE_FL_ENCODING)) {
> +				params->encoding = EXT4_ENC_UTF8_12_1;
> +				params->set_flags |= EXT4_TUNE_FL_ENCODING;
>  			}
> -			if (!(params.set_flags & EXT4_TUNE_FL_ENCODING_FLAGS)) {
> -				params.encoding_flags = 0;
> -				params.set_flags |= EXT4_TUNE_FL_ENCODING_FLAGS;
> +			if (!(params->set_flags &
> +			      EXT4_TUNE_FL_ENCODING_FLAGS)) {
> +				params->encoding_flags = 0;
> +				params->set_flags |=
> +					EXT4_TUNE_FL_ENCODING_FLAGS;
>  			}
>  		}

[Severity: High]
This problem wasn't introduced by this patch, but when
EXT4_FEATURE_INCOMPAT_CASEFOLD is enabled online via
EXT4_IOC_SET_TUNE_SB_PARAM [1], does anything initialize
sb->s_encoding in memory?

ext4_sb_setparams() sets EXT4_FEATURE_INCOMPAT_CASEFOLD in the superblock
without loading the unicode map into sb->s_encoding.  Subsequent
EXT4_IOC_SETFLAGS calls can then set EXT4_CASEFOLD_FL on a directory, and a
lookup in that directory calls ext4_fname_setup_ci_filename() ->
utf8_casefold(dir->i_sb->s_encoding, ...), dereferencing a NULL
sb->s_encoding pointer.

[ ... ]
> +void ext4_swap_inode_data_test(struct inode *inode1, struct inode *inode2)
> +{
> +	swap_inode_data(inode1, inode2);
> +}
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_swap_inode_data_test);

[Severity: High]
This problem wasn't introduced by this patch, but can swap_inode_data()
cause torn reads of i_version on 32-bit architectures [2]?

fs/ext4/ioctl.c:swap_inode_data() {
	...
	swap(inode1->i_version, inode2->i_version);
	...
}

Because i_version is an atomic64_t, exchanging it with swap() performs plain
struct assignment instead of using atomic64 accessors, allowing concurrent
lockless readers such as inode_peek_iversion_raw() to observe torn values on
32-bit SMP systems.

[1] https://sashiko.dev/#/bug/linux-4eae7256-2707-4887-b442-c774f5662e2b
[2] https://sashiko.dev/#/bug/linux-14d568d4-dbf5-4a52-9af4-757563dc37e3

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