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
