Re: [PATCH 2/5] module/dups: Inform duplicate requests about the result directly

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Dropping `kmod_dup_mutex` prematurely without acquiring a reference leads to a Use-After-Free and data races.

Pre-existing issues:
- [High] A `request_module_nowait` call can erroneously complete a concurrent `request_module` duplicate tracking object prematurely.
--

--- Patch [2]: [PATCH 2/5] module/dups: Inform duplicate requests about the result directly ---
commit 5b4a732b8aa814f2e4a78fd3e59bb299a50f8a41
Author: Petr Pavlu <petr.pavlu@suse.com>

module/dups: Inform duplicate requests about the result directly

When kmod_dup_request_announce() announces the completion of
a request_module() call to duplicate waiters, it queues a work item to
invoke kmod_dup_request_complete(), and only that function calls
complete_all().

This adds an arbitrary delay that is unnecessary and provides little
benefit. Call complete_all() directly from kmod_dup_request_announce()
instead.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>

> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> index e1828e865edde..f0bdd7560d7e7 100644
> --- a/kernel/module/dups.c
> +++ b/kernel/module/dups.c

[ ... ]

> @@ -246,23 +221,22 @@ void kmod_dup_request_announce(char *module_name, int ret)
>  	 * that is owned by the current task.
>  	 */
>  	kmod_req = kmod_dup_request_lookup(module_name);
> +	mutex_unlock(&kmod_dup_mutex);
> +
>  	if (!kmod_req)
> -		goto out;
> +		return;
>  
>  	kmod_req->dup_ret = ret;

[Severity: High]
Does unlocking kmod_dup_mutex here leave kmod_req unprotected?

If a thread is preempted after unlocking the mutex but before writing to
kmod_req->dup_ret, could delete_work execute, drop the refcount to zero,
and free kmod_req? This would result in a use-after-free when the preempted
thread resumes and attempts to write to kmod_req->dup_ret or call
complete_all(). Should we hold the mutex longer or acquire a reference?

> +	/* Inform all the duplicate waiters to check the return value. */
> +	complete_all(&kmod_req->first_req_done);

[Severity: High]
This is a pre-existing issue, but can this prematurely complete a concurrent
request_module() call?

When request_module_nowait() is called, it skips adding a tracking object to
the dup_kmod_reqs list, but continues to execute call_modprobe() and
unconditionally calls kmod_dup_request_announce().

If a concurrent request_module() call had created a tracking object for the
same module, the nowait thread's lookup will find it and call complete_all(),
prematurely waking up all duplicate waiters before the original modprobe has
actually finished. Does this function need to verify the caller owns the
request before completing it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720121621.750661-1-petr.pavlu@suse.com?part=2