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, int > ret) > > kmod_req->dup_ret = ret; > > + /* 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. > > out: > mutex_unlock(&kmod_dup_mutex); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
