Re: [PATCH v3 3/6] module/dups: Avoid unnecessary kmod_dup_req allocations

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

Pre-existing issues:
- [High] A `request_module_nowait()` call can prematurely complete a concurrent `request_module(wait=true)` request, causing synchronous waiters to proceed before the module is actually loaded.
--

--- Patch [3]: [PATCH v3 3/6] module/dups: Avoid unnecessary kmod_dup_req allocations ---
commit c39253dbe61d24363b967a87af56bd7ef5c557ff
Author: Petr Pavlu <[email protected]>

module/dups: Avoid unnecessary kmod_dup_req allocations

This commit defers the allocation of kmod_dup_req until it is actually
needed, instead of preallocating it before taking the mutex. It introduces
a helper alloc_kmod_req() to simplify the initialization process.

> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> --- a/kernel/module/dups.c
> +++ b/kernel/module/dups.c
[ ... ]
> @@ -138,7 +141,6 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
>  		 * If a prior request to this one came through with
>  		 * request_module() though, then a request_module_nowait()
>  		 * would benefit from duplicate detection.
>  		 */
>  		if (!wait) {
> -			kfree(new_kmod_req);
>  			pr_debug("New request_module_nowait() for %s -- cannot track duplicates for this request\n", module_name);
>  			mutex_unlock(&kmod_dup_mutex);
>  			return false;

[Severity: High]
This is a pre-existing issue, but does a request_module_nowait() call risk
prematurely completing a concurrent request_module() call that is waiting?

If Thread B calls request_module_nowait("foo"), it doesn't create a
tracking entry because wait is false. If Thread A subsequently calls
request_module("foo") with wait=true, it creates the tracking entry and
blocks for processing. If Thread C then calls request_module("foo") with
wait=true, it finds Thread A's entry and blocks waiting for it.

Because Thread B used nowait, its execution finishes quickly (waiting only
for the modprobe binary to exec) and returns 0. It then unconditionally
announces completion:

kernel/module/dups.c:kmod_dup_request_announce() {
        ...
        kmod_req = kmod_dup_request_lookup(module_name);
        if (!kmod_req || completion_done(&kmod_req->first_req_done)) {
                mutex_unlock(&kmod_dup_mutex);
                return;
        }

        kmod_req->dup_ret = ret;

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

Won't this find Thread A's tracking entry, assign dup_ret = 0, and wake up
Thread C prematurely while the module is still being loaded in the
background by Thread A?

If Thread C receives a success return value before the module is actually
fully loaded and initialized, could this cause drivers or subsystems to
attempt to use uninitialized module symbols or hardware features?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3