Thread (12 messages) 12 messages, 2 authors, 8d ago

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

From: Petr Pavlu <petr.pavlu@suse.com>
Date: 2026-07-20 13:28:49
Also in: sashiko-reviews

On 7/20/26 2:25 PM, sashiko-bot@kernel.org wrote:
quoted hunk ↗ jump to hunk
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 [off-list ref]

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.
quoted
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
[ ... ]
quoted
@@ -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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help