Re: [PATCH v3 1/6] module/dups: Inform duplicate requests about the result directly

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 [off-list ref]

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.

[ ... ]

[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/20260806-module-stats-rcu-v3-0-2b0331106f00@suse.com?part=1