Re: [PATCH 3/5] module/dups: Use scoped guards for kmod_dup_mutex

On 7/20/26 2:25 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Mixing goto-based cleanup labels with scope-based cleanup (scoped_guard) in the same function.
> --
> 
> --- Patch [3]: [PATCH 3/5] module/dups: Use scoped guards for kmod_dup_mutex ---
> commit 3299ca2ac8ae46392f412ae7c186f4907e7f8201
> Author: Petr Pavlu <[email protected]>
> 
> module/dups: Use scoped guards for kmod_dup_mutex
> 
> This patch replaces explicit mutex_lock/unlock calls for kmod_dup_mutex
> with scoped_guard to shorten the code and clarify the lock's scope.
> 
>> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
>> index f0bdd7560d7e7..a62619ec057ed 100644
>> --- a/kernel/module/dups.c
>> +++ b/kernel/module/dups.c
> 
> [ ... ]
> 
>> @@ -122,10 +121,13 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
>>  	INIT_DELAYED_WORK(&new_kmod_req->delete_work, kmod_dup_request_delete);
>>  	init_completion(&new_kmod_req->first_req_done);
>>  
>> -	mutex_lock(&kmod_dup_mutex);
>> +	scoped_guard(mutex, &kmod_dup_mutex) {
>> +		kmod_req = kmod_dup_request_lookup(module_name);
>> +		if (kmod_req) {
>> +			get_kmod_req(kmod_req);
>> +			goto dup_req;
>> +		}
>>  
>> -	kmod_req = kmod_dup_request_lookup(module_name);
>> -	if (!kmod_req) {
> 
> [Severity: Low]
> Does this mix scope-based cleanup with goto-based cleanup in the same
> function?
> 
> Looking further down in kmod_dup_request_exists_wait(), there are still
> several goto statements that jump to an out label for manual cleanup:
> 
> 	if (!wait) {
> 		/* ... */
> 		*dup_ret = 0;
> 		goto out;
> 	}
> 	
> 	ret = wait_for_completion_state(&kmod_req->first_req_done,
> 					TASK_KILLABLE);
> 	if (ret) {
> 		*dup_ret = ret;
> 		goto out;
> 	}
> 
> 	*dup_ret = kmod_req->dup_ret;
> 
> out:
> 	put_kmod_req(kmod_req);
> 	return true;
> 
> According to include/linux/cleanup.h, the expectation is that usage of
> "goto" and cleanup helpers is never mixed in the same function. All resources 
> that need a "goto" cleanup should either be converted to scope-based cleanup, 
> or none of them should.
> 
> Should put_kmod_req() be converted to use scope-based cleanup as well, or 
> should this function avoid using scoped_guard() to keep the cleanup pattern
> consistent?
> 

Fair, I'll clean this up.

Sashiko also reported the same issue in a reply to patch #5.

-- Petr