Re: [PATCH 23/27] ext4: expand KUnit tests in fs/ext4/extents-test.c for file range operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] ext4: uninitialized invalidate_lock in ext_init_mock_inode()
- [Low] ext4: use-after-free of sb->s_bdev after deactivate_super() in
  extents_kunit_exit()
- [Low] ext4: i_data_sem and path leak on assertion abort in
  ext_expect_extent_at()
--

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

ext4: expand KUnit tests in fs/ext4/extents-test.c for file range operations

This commit expands the ext4 extents KUnit test suite to cover
ext4_fallocate(), ext4_zero_range(), ext4_collapse_range(),
ext4_insert_range(), unwritten extent conversion helpers, ext4_fiemap(),
and ext4_get_es_cache().

> diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
> index 7565a8aa4e106..4f1746be323ff 100644
> --- a/fs/ext4/extents-test.c
> +++ b/fs/ext4/extents-test.c

[ ... ]

> @@ -200,20 +210,29 @@ static void ext_init_mock_inode(struct ext4_inode_info *ei,
>  	inode->i_sb = sb;
>  	inode->i_ino = ino;
>  	inode->i_mode = S_IFREG | 0644;
> +	set_nlink(inode, 1);
>  	inode->i_blkbits = sb->s_blocksize_bits;
> -	inode->i_flags |= S_NOQUOTA;
> +	inode->i_flags |= S_NOQUOTA | S_NOSEC;
>  	inode->i_mapping = &inode->i_data;
> +	inode_set_iversion(inode, 1);
>  
>  	spin_lock_init(&inode->i_lock);
>  	init_rwsem(&inode->i_rwsem);
> -	init_rwsem(&inode->i_data.invalidate_lock);
> +	address_space_init_once(&inode->i_data);
> +	inode->i_data.host = inode;
>  	inode_state_set_raw(inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);

[Severity: Medium]
Does replacing init_rwsem(&inode->i_data.invalidate_lock) with
address_space_init_once(&inode->i_data) leave invalidate_lock
uninitialized?

address_space_init_once() zeroes inode->i_data and initializes only
i_pages, i_mmap_rwsem, i_private_lock, and i_mmap; in the VFS,
invalidate_lock is initialized separately in inode_init_always_gfp().
When the new fallocate tests call ext4_fallocate() with punch hole, zero
range, collapse range, or insert range, filemap_invalidate_lock() acquires
this zeroed rw_semaphore, which triggers lockdep or CONFIG_DEBUG_RWSEMS
warnings and deadlocks on CONFIG_PREEMPT_RT kernels.  Keeping
init_rwsem(&inode->i_data.invalidate_lock) after address_space_init_once()
avoids this.

[ ... ]

> @@ -267,6 +286,7 @@ static void extents_kunit_exit(struct kunit *test)
>  
>  	kfree(k_ctx.meta_blocks);
>  	kfree(sbi->s_es);
> +	kfree(sb->s_bdev->bd_queue);
>  	kfree(sb->s_bdev->bd_mapping);
>  	kfree(sb->s_bdev);
>  	kfree(sbi);

[Severity: Low]
Can dereferencing sb->s_bdev here use sb after it has been freed?

deactivate_super(sb) is called just above this hunk, which drops the final
reference on sb and schedules its destruction via RCU and
destroy_super_work(), unlike the error path in extents_kunit_init() which
frees sb->s_bdev->bd_queue, sb->s_bdev->bd_mapping, and sb->s_bdev before
deactivating the superblock.

[ ... ]

> +static void ext_expect_extent_at(struct kunit *test, struct inode *inode,
> +				 ext4_lblk_t lblk, unsigned short exp_len,
> +				 ext4_fsblk_t exp_pblk, bool exp_unwritten)
> +{
> +	struct ext4_ext_path *path;
> +	struct ext4_extent *ex;
> +
> +	down_read(&EXT4_I(inode)->i_data_sem);
> +	path = ext4_find_extent(inode, lblk, NULL, EXT4_EX_NOCACHE);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR(path));
> +	ex = path[path->p_depth].p_ext;
> +	KUNIT_ASSERT_NOT_NULL(test, ex);
> +
> +	KUNIT_EXPECT_EQ(test, le32_to_cpu(ex->ee_block), lblk);
> +	KUNIT_EXPECT_EQ(test, ext4_ext_get_actual_len(ex), exp_len);
> +	KUNIT_EXPECT_EQ(test, ext4_ext_pblock(ex), exp_pblk);
> +	KUNIT_EXPECT_EQ(test, !!ext4_ext_is_unwritten(ex), exp_unwritten);
> +
> +	ext4_free_ext_path(path);
> +	up_read(&EXT4_I(inode)->i_data_sem);
> +}

[Severity: Low]
If KUNIT_ASSERT_FALSE() or KUNIT_ASSERT_NOT_NULL() fails in
ext_expect_extent_at(), does the test abort with EXT4_I(inode)->i_data_sem
still held for read and path leaked?

Because KUNIT_ASSERT_* immediately terminates the test kthread,
extents_kunit_exit() -> ext_cleanup_inode() will then deadlock on
down_write(&ei->i_data_sem).

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