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 <[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. Signed-off-by: Petr Pavlu <[email protected]> > 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/[email protected]?part=2
