[PATCH] module: dups: don't hold mutex across synchronize_rcu()

[PATCH] module: dups: don't hold mutex across synchronize_rcu()

[Naveen Kumar Chaudhary]

kmod_dup_request_delete() calls synchronize_rcu() while holding
kmod_dup_mutex. This is unnecessary since list_del_rcu() only needs
the mutex for list manipulation, and the RCU grace period just needs
to complete before kfree(). Holding the mutex across the grace period
blocks all other kmod_dup_mutex users for the duration, causing
unnecessary latency.

Move synchronize_rcu() after the mutex is released.

Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
---
 kernel/module/dups.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index 1d720a5311ba..46e74c06ed52 100644
--- a/kernel/module/dups.c
+++ b/kernel/module/dups.c
@@ -88,8 +88,8 @@ static void kmod_dup_request_delete(struct work_struct *work)
 	 */
 	mutex_lock(&kmod_dup_mutex);
 	list_del_rcu(&kmod_req->list);
-	synchronize_rcu();
 	mutex_unlock(&kmod_dup_mutex);
+	synchronize_rcu();
 	kfree(kmod_req);
 }
 
-- 
2.43.0

Re: [PATCH] module: dups: don't hold mutex across synchronize_rcu()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free due to missing reference counting on kmod_req.
--

commit 6715d39010e803da11ebd165eb2b1fa9278aa3d7
Author: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>

module: dups: don't hold mutex across synchronize_rcu()

This commit optimizes kmod_dup_request_delete() by moving synchronize_rcu()
outside the kmod_dup_mutex critical section. This prevents blocking other
mutex users during the RCU grace period.

> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> index 1d720a5311ba0..46e74c06ed52d 100644
> --- a/kernel/module/dups.c
> +++ b/kernel/module/dups.c
> @@ -88,8 +88,8 @@ static void kmod_dup_request_delete(struct work_struct *work)
>  	 */
>  	mutex_lock(&kmod_dup_mutex);
>  	list_del_rcu(&kmod_req->list);
> -	synchronize_rcu();
>  	mutex_unlock(&kmod_dup_mutex);
> +	synchronize_rcu();
>  	kfree(kmod_req);

[Severity: High]
This is a pre-existing issue, but does this code leave kmod_req vulnerable
to a use-after-free since waiters don't hold a reference count?

In kmod_dup_request_exists_wait(), a duplicate requester drops kmod_dup_mutex
and blocks on wait_for_completion_state(&kmod_req->first_req_done). If the
original thread finishes loading and calls kmod_dup_request_complete(), it
queues this delete_work with an arbitrary 60-second delay.

If the waiter is preempted or otherwise delayed for more than 60 seconds
(either before blocking, or after waking up but before reading
kmod_req->dup_ret), kmod_dup_request_delete() will free kmod_req here.

The synchronize_rcu() call does not protect the waiter because the waiter
sleeps in wait_for_completion_state() and cannot hold an RCU read lock.
When the waiter resumes, it could attempt lock operations on the freed
completion structure's waitqueue lock, or read kmod_req->dup_ret, resulting
in a use-after-free.

Would it be safer to add explicit reference counting to kmod_req to ensure
it remains allocated until all waiters have resumed and finished reading?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ajydyxgaea27rhcopp5eauji24znotu65d2b4uw344yvmwcc6f@7l5re6f2xcuk?part=1