Re: [PATCH v2 1/6] module/dups: Inform duplicate requests about the result directly

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

Pre-existing issues:
- [High] Lack of reference counting on `kmod_dup_req` allows Use-After-Free=
 vulnerabilities when duplicate waiters or concurrent announcers race with =
the 60-second deletion timer.
--

--- Patch [1]: [PATCH v2 1/6] module/dups: Inform duplicate requests about =
the result directly ---
commit a68253df788049311e521e9217563df8d2aeb41e
Author: Petr Pavlu <[email protected]>

    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.

> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> index 1d720a5311ba0..b063bc423aa81 100644
> --- a/kernel/module/dups.c
> +++ b/kernel/module/dups.c

[ ... ]

> @@ -230,17 +205,17 @@ void kmod_dup_request_announce(char *module_name, i=
nt ret)
> =20
>  	kmod_req->dup_ret =3D ret;
> =20
> +	/* Inform all duplicate waiters to check the return value. */
> +	complete_all(&kmod_req->first_req_done);
> +
>  	/*
> -	 * If we complete() here we may allow duplicate threads
> -	 * to continue before the first one that submitted the
> -	 * request. We're in no rush also, given that each and
> -	 * every bounce back to userspace is slow we avoid that
> -	 * with a slight delay here. So queueue up the completion
> -	 * and let duplicates suffer, just wait a tad bit longer.
> -	 * There is no rush. But we also don't want to hold the
> -	 * caller up forever or introduce any boot delays.
> +	 * Now that we have allowed prior request_module() calls to go on
> +	 * with life, let's schedule deleting this entry. We don't have
> +	 * to do it right away, but we *eventually* want to do it so to not
> +	 * let this linger forever as this is just a boot optimization for
> +	 * possible abuses of vmalloc() incurred by finit_module() thrashing.
>  	 */
> -	queue_work(system_dfl_wq, &kmod_req->complete_work);
> +	queue_delayed_work(system_dfl_wq, &kmod_req->delete_work, 60 * HZ);

[Severity: High]
This is a pre-existing issue, but could this delayed work cause a
use-after-free since kmod_dup_req lacks reference counting?

If a duplicate request thread waits on first_req_done in
kmod_dup_request_exists_wait() without holding a reference, and is preempted
for over 60 seconds after waking up, the deletion worker could call kfree()
on kmod_req in kmod_dup_request_delete(). When the waiter resumes, reading
kmod_req->dup_ret would result in a use-after-free.

Additionally, could queue_delayed_work() here race with an actively
executing teardown? If two threads concurrently call
kmod_dup_request_announce() for the same module, and the second thread calls
it exactly when the 60-second timer fires, it could re-arm the timer while
the first worker thread clears the pending bit but blocks on
kmod_dup_mutex. The second thread would release the mutex, the worker would
proceed to free kmod_req, and the re-armed timer would later fire on freed
memory.

> =20
>  out:
>  	mutex_unlock(&kmod_dup_mutex);

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260723-module-sta=
[email protected]?part=3D1