Re: [PATCH v4 3/5] ext4: fix the error handling process in extents_kunit_init).

["yebin (H)" <yebin10@huawei.com>]

On 2026/3/20 9:56, Ritesh Harjani wrote:
> Ye Bin <yebin@huaweicloud.com> writes:
>
>> From: Ye Bin <yebin10@huawei.com>
>>
>> The error processing in extents_kunit_init() is improper, causing
>> resource leakage.
>> Reconstruct the error handling process to prevent potential resource
>> leaks
>>
>> Fixes: cb1e0c1d1fad ("ext4: kunit tests for extent splitting and conversion")
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
>> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>   fs/ext4/extents-test.c | 43 ++++++++++++++++++++++++++++++------------
>>   1 file changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
>> index 3d4663d99eb1..4ce3f81f6409 100644
>> --- a/fs/ext4/extents-test.c
>> +++ b/fs/ext4/extents-test.c
>> @@ -225,33 +225,37 @@ static int extents_kunit_init(struct kunit *test)
>>   		(struct kunit_ext_test_param *)(test->param_value);
>>   	int err;
>>
>> -	sb = sget(&ext_fs_type, NULL, ext_set, 0, NULL);
>> -	if (IS_ERR(sb))
>> -		return PTR_ERR(sb);
>> -
>> -	sb->s_blocksize = 4096;
>> -	sb->s_blocksize_bits = 12;
>> -
>>   	sbi = kzalloc_obj(struct ext4_sb_info);
>>   	if (sbi == NULL)
>>   		return -ENOMEM;
>>
>> +	sb = sget(&ext_fs_type, NULL, ext_set, 0, NULL);
>> +	if (IS_ERR(sb)) {
>> +		kfree(sbi);
>> +		return PTR_ERR(sb);
>> +	}
>> +
>>   	sbi->s_sb = sb;
>>   	sb->s_fs_info = sbi;
>>
>> +	sb->s_blocksize = 4096;
>> +	sb->s_blocksize_bits = 12;
>> +
>>   	if (!param || !param->disable_zeroout)
>>   		sbi->s_extent_max_zeroout_kb = 32;
>>
>>   	/* setup the mock inode */
>>   	k_ctx.k_ei = kzalloc_obj(struct ext4_inode_info);
>> -	if (k_ctx.k_ei == NULL)
>> -		return -ENOMEM;
>> +	if (k_ctx.k_ei == NULL) {
>> +		err = -ENOMEM;
>> +		goto out_deactivate;
>> +	}
>>   	ei = k_ctx.k_ei;
>>   	inode = &ei->vfs_inode;
>>
>>   	err = ext4_es_register_shrinker(sbi);
>>   	if (err)
>> -		return err;
>> +		goto out_deactivate;
>
> Even though the patch looks ok, but still wanted to check if ...
>
> Do you think we can move ext4_es_register_shrinker() before setting up
> the mock inode and on error we simply return err?
> That way, there won't be any ambiguity in the error handling for calling
> ext4_es_unregister_shrinker()?
Sorry, I've been a bit busy lately. The idea is good, and I will send a
new version of the patch.
>
>>
>>   	ext4_es_init_tree(&ei->i_es_tree);
>>   	rwlock_init(&ei->i_es_lock);
>> @@ -267,8 +271,10 @@ static int extents_kunit_init(struct kunit *test)
>>   	inode->i_sb = sb;
>>
>>   	k_ctx.k_data = kzalloc(EXT_DATA_LEN * 4096, GFP_KERNEL);
>> -	if (k_ctx.k_data == NULL)
>> -		return -ENOMEM;
>> +	if (k_ctx.k_data == NULL) {
>> +		err = -ENOMEM;
>> +		goto out_deactivate;
>> +	}
>>
>>   	/*
>>   	 * set the data area to a junk value
>> @@ -313,6 +319,19 @@ static int extents_kunit_init(struct kunit *test)
>>   	up_write(&sb->s_umount);
>>
>>   	return 0;
>> +
>> +out_deactivate:
>> +	kfree(k_ctx.k_ei);
>> +	k_ctx.k_ei = NULL;
>> +
>> +	kfree(k_ctx.k_data);
>> +	k_ctx.k_data = NULL;
>> +
>> +	ext4_es_unregister_shrinker(sbi);
>
> -ritesh
>
>
> .
>