Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Re-arming `delete_work` while it is currently executing leads to a Use-After-Free and timer corruption. - [High] Waiters sleep in `wait_for_completion_state()` without holding a reference to `kmod_req`, allowing `delete_work` to free the structure out from under them. -- --- Patch [1]: [PATCH v3 1/6] module/dups: Inform duplicate requests about the result directly --- commit b996b7173ceb4926f129328065b0224c2ccfe2d2 Author: Petr Pavlu <[email protected]> module/dups: Inform duplicate requests about the result directly This patch removes the complete_work workqueue item that was used to delay the completion signal. It updates kmod_dup_request_announce() to call complete_all() and then immediately queue delete_work to clean up the entry. > diff --git a/kernel/module/dups.c b/kernel/module/dups.c > index 8b6f8f177de4f..45080f451e5c6 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); > + [Severity: High] This isn't a bug introduced by this patch, but could this lead to a use-after-free if a waiter thread is severely delayed after being woken up? Waiters in kmod_dup_request_exists_wait() wait on this completion without taking a reference to kmod_req: kernel/module/dups.c:kmod_dup_request_exists_wait() { ... wait_for_completion_state(&kmod_req->first_req_done, ...); ... *dup_ret = kmod_req->dup_ret; ... } If a woken waiter is preempted, heavily throttled, or the system is suspended for more than 60 seconds before it can return and read kmod_req->dup_ret, delete_work will run and free kmod_req, causing a use-after-free when the waiter resumes. > /* > - * 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: Critical] This is a pre-existing issue, but could this queue_delayed_work() call lead to a use-after-free and timer corruption if it races with an executing delete_work? If a previous delete_work worker wakes up, clears the WORK_STRUCT_PENDING bit, and blocks on kmod_dup_mutex: Worker Thread: (clears WORK_STRUCT_PENDING) mutex_lock(&kmod_dup_mutex); // blocked Concurrent module load: kmod_dup_request_announce() mutex_lock(&kmod_dup_mutex); ... queue_delayed_work(..., &kmod_req->delete_work, 60 * HZ); mutex_unlock(&kmod_dup_mutex); Because the pending bit was cleared by the worker, queue_delayed_work() will successfully re-arm the timer. When the worker thread acquires the mutex, it will unlink kmod_req and unconditionally call kfree(). When the re-armed timer expires 60 seconds later, the kernel will dereference the timer_list from freed memory, resulting in a panic. > > out: > mutex_unlock(&kmod_dup_mutex); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
