[PATCH v3 0/6] module/dups: Fix a use-after-free and improve the code

[PATCH v3 0/6] module/dups: Fix a use-after-free and improve the code

[Petr Pavlu]

Simplify how the kmod dups code informs duplicate requests about the
result, fix a potential use-after-free when waiting on a duplicate request,
and make several general improvements to the code.

Note that the use-after-free fix depends on the code simplification
implemented in the first patch.

---
Changes in v3:
- Add received Reviewed-by tags.
- Rebase the series on modules-next.
- Link to v2: https://patch.msgid.link/20260723-module-stats-rcu-v2-0-bd582261fae3@suse.com

Changes in v2:
- Fix handling of concurrent request_module_nowait() and request_module()
  calls.
- Avoid unnecessary kmod_dup_req allocations.
- Avoid mixing goto with scope-based cleanup.
- Link to v1: https://patch.msgid.link/20260720121621.750661-1-petr.pavlu@suse.com

---
Petr Pavlu (6):
      module/dups: Inform duplicate requests about the result directly
      module/dups: Fix use-after-free in kmod_dup_req lifetime handling
      module/dups: Avoid unnecessary kmod_dup_req allocations
      module/dups: Use scope-based cleanup helpers
      module/dups: Use strcmp() to compare module names
      module/dups: Clean up includes

 kernel/module/dups.c | 169 +++++++++++++++++++++++----------------------------
 1 file changed, 77 insertions(+), 92 deletions(-)
---
base-commit: 2600d43b235b029d0ba4f2d3cc71e2137114fde3
change-id: 20260722-module-stats-rcu-ff0dfc986be6

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

[Petr Pavlu]

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 <petr.pavlu@suse.com>
---
 kernel/module/dups.c | 43 +++++++++----------------------------------
 1 file changed, 9 insertions(+), 34 deletions(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index 8b6f8f177de4..45080f451e5c 100644
--- a/kernel/module/dups.c
+++ b/kernel/module/dups.c
@@ -48,7 +48,6 @@ struct kmod_dup_req {
 	struct list_head list;
 	char name[MODULE_NAME_LEN];
 	struct completion first_req_done;
-	struct work_struct complete_work;
 	struct delayed_work delete_work;
 	int dup_ret;
 };
@@ -93,29 +92,6 @@ static void kmod_dup_request_delete(struct work_struct *work)
 	kfree(kmod_req);
 }
 
-static void kmod_dup_request_complete(struct work_struct *work)
-{
-	struct kmod_dup_req *kmod_req;
-
-	kmod_req = container_of(work, struct kmod_dup_req, complete_work);
-
-	/*
-	 * This will ensure that the kernel will let all the waiters get
-	 * informed its time to check the return value. It's time to
-	 * go home.
-	 */
-	complete_all(&kmod_req->first_req_done);
-
-	/*
-	 * 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_delayed_work(system_dfl_wq, &kmod_req->delete_work, 60 * HZ);
-}
-
 bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 {
 	struct kmod_dup_req *kmod_req, *new_kmod_req;
@@ -130,7 +106,6 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 		return false;
 
 	strscpy(new_kmod_req->name, module_name);
-	INIT_WORK(&new_kmod_req->complete_work, kmod_dup_request_complete);
 	INIT_DELAYED_WORK(&new_kmod_req->delete_work, kmod_dup_request_delete);
 	init_completion(&new_kmod_req->first_req_done);
 
@@ -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);
 
 out:
 	mutex_unlock(&kmod_dup_mutex);

-- 
2.55.0

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

[sashiko-bot]

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 <petr.pavlu@suse.com>

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

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

[Petr Pavlu]

On 8/6/26 11:25 AM, sashiko-bot@kernel.org wrote:
> 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.

The series fixes these use-after-free problems in patch 2.

[PATCH v3 2/6] module/dups: Fix use-after-free in kmod_dup_req lifetime handling

[Petr Pavlu]

The kmod dups code uses RCU to ensure that a kmod_dup_req instance is freed
only after it is no longer referenced. When releasing an instance, the
kmod_dup_request_delete() function removes the kmod_dup_req from the
dup_kmod_reqs list, waits via synchronize_rcu() and finally frees it.
However, this doesn't work correctly because parallel users referencing the
instance in kmod_dup_request_exists_wait() don't enter an RCU read-side
critical section. This can result in a use-after-free.

The kmod_dup_request_exists_wait() function may need to hold a valid
reference to a kmod_dup_req instance across a blocking wait until the
corresponding modprobe command completes. This makes it unsuitable for RCU.

Fix the issue by changing the lifecycle management of kmod_dup_req to use
reference counting.

Fixes: 8660484ed1cf ("module: add debugging auto-load duplicate module support")
Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/dups.c | 56 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index 45080f451e5c..db7377229703 100644
--- a/kernel/module/dups.c
+++ b/kernel/module/dups.c
@@ -30,6 +30,7 @@
 #include <linux/ptrace.h>
 #include <linux/async.h>
 #include <linux/uaccess.h>
+#include <linux/refcount.h>
 
 #include "internal.h"
 
@@ -38,13 +39,12 @@
 static bool enable_dups_trace = IS_ENABLED(CONFIG_MODULE_DEBUG_AUTOLOAD_DUPS_TRACE);
 module_param(enable_dups_trace, bool_enable_only, 0644);
 
-/*
- * Protects dup_kmod_reqs list, adds / removals with RCU.
- */
+/* A mutex-protected list of active kmod requests. */
 static DEFINE_MUTEX(kmod_dup_mutex);
 static LIST_HEAD(dup_kmod_reqs);
 
 struct kmod_dup_req {
+	refcount_t refcount;
 	struct list_head list;
 	char name[MODULE_NAME_LEN];
 	struct completion first_req_done;
@@ -52,12 +52,24 @@ struct kmod_dup_req {
 	int dup_ret;
 };
 
+static void get_kmod_req(struct kmod_dup_req *kmod_req)
+{
+	refcount_inc(&kmod_req->refcount);
+}
+
+static void put_kmod_req(struct kmod_dup_req *kmod_req)
+{
+	if (refcount_dec_and_test(&kmod_req->refcount))
+		kfree(kmod_req);
+}
+
 static struct kmod_dup_req *kmod_dup_request_lookup(char *module_name)
 {
 	struct kmod_dup_req *kmod_req;
 
-	list_for_each_entry_rcu(kmod_req, &dup_kmod_reqs, list,
-				lockdep_is_held(&kmod_dup_mutex)) {
+	lockdep_assert_held(&kmod_dup_mutex);
+
+	list_for_each_entry(kmod_req, &dup_kmod_reqs, list) {
 		if (strlen(kmod_req->name) == strlen(module_name) &&
 		    !memcmp(kmod_req->name, module_name, strlen(module_name))) {
 			return kmod_req;
@@ -86,10 +98,10 @@ static void kmod_dup_request_delete(struct work_struct *work)
 	 * just returning 0.
 	 */
 	mutex_lock(&kmod_dup_mutex);
-	list_del_rcu(&kmod_req->list);
-	synchronize_rcu();
+	list_del(&kmod_req->list);
 	mutex_unlock(&kmod_dup_mutex);
-	kfree(kmod_req);
+
+	put_kmod_req(kmod_req);
 }
 
 bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
@@ -105,6 +117,7 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 	if (!new_kmod_req)
 		return false;
 
+	refcount_set(&new_kmod_req->refcount, 1);
 	strscpy(new_kmod_req->name, module_name);
 	INIT_DELAYED_WORK(&new_kmod_req->delete_work, kmod_dup_request_delete);
 	init_completion(&new_kmod_req->first_req_done);
@@ -136,10 +149,12 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 		 * keep tab on duplicates later.
 		 */
 		pr_debug("New request_module() for %s\n", module_name);
-		list_add_rcu(&new_kmod_req->list, &dup_kmod_reqs);
+		list_add(&new_kmod_req->list, &dup_kmod_reqs);
 		mutex_unlock(&kmod_dup_mutex);
 		return false;
 	}
+
+	get_kmod_req(kmod_req);
 	mutex_unlock(&kmod_dup_mutex);
 
 	/* We are dealing with a duplicate request now */
@@ -169,7 +184,7 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 		 * calls bail out right away.
 		 */
 		*dup_ret = 0;
-		return true;
+		goto out;
 	}
 
 	/*
@@ -184,12 +199,14 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 					TASK_KILLABLE);
 	if (ret) {
 		*dup_ret = ret;
-		return true;
+		goto out;
 	}
 
 	/* Now the duplicate request has the same exact return value as the first request */
 	*dup_ret = kmod_req->dup_ret;
 
+out:
+	put_kmod_req(kmod_req);
 	return true;
 }
 
@@ -199,15 +216,25 @@ void kmod_dup_request_announce(char *module_name, int ret)
 
 	mutex_lock(&kmod_dup_mutex);
 
+	/*
+	 * Look for a kmod_dup_req previously added in
+	 * kmod_dup_request_exists_wait(). Note that a request_module_nowait()
+	 * without its own kmod_dup_req entry can announce a result of
+	 * a concurrent request_module() call.
+	 */
 	kmod_req = kmod_dup_request_lookup(module_name);
-	if (!kmod_req)
-		goto out;
+	if (!kmod_req || completion_done(&kmod_req->first_req_done)) {
+		mutex_unlock(&kmod_dup_mutex);
+		return;
+	}
 
 	kmod_req->dup_ret = ret;
 
 	/* Inform all duplicate waiters to check the return value. */
 	complete_all(&kmod_req->first_req_done);
 
+	mutex_unlock(&kmod_dup_mutex);
+
 	/*
 	 * Now that we have allowed prior request_module() calls to go on
 	 * with life, let's schedule deleting this entry. We don't have
@@ -216,7 +243,4 @@ void kmod_dup_request_announce(char *module_name, int ret)
 	 * possible abuses of vmalloc() incurred by finit_module() thrashing.
 	 */
 	queue_delayed_work(system_dfl_wq, &kmod_req->delete_work, 60 * HZ);
-
-out:
-	mutex_unlock(&kmod_dup_mutex);
 }

-- 
2.55.0

[PATCH v3 3/6] module/dups: Avoid unnecessary kmod_dup_req allocations

[Petr Pavlu]

The kmod dups code preallocates kmod_dup_req before taking kmod_dup_mutex
to avoid allocating memory while holding the lock. This provides little
benefit, since the allocation is fast and can safely be done under the
lock. On the other hand, it leads to unnecessary allocations when the
request turns out to be a duplicate and slightly complicates the code.

Allocate kmod_dup_req only when needed and introduce a helper function
alloc_kmod_req() to initialize the structure.

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/dups.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index db7377229703..276016051ad2 100644
--- a/kernel/module/dups.c
+++ b/kernel/module/dups.c
@@ -104,28 +104,31 @@ static void kmod_dup_request_delete(struct work_struct *work)
 	put_kmod_req(kmod_req);
 }
 
-bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
+static struct kmod_dup_req *alloc_kmod_req(const char *module_name)
 {
-	struct kmod_dup_req *kmod_req, *new_kmod_req;
-	int ret;
+	struct kmod_dup_req *kmod_req = kzalloc_obj(*kmod_req);
 
-	/*
-	 * Pre-allocate the entry in case we have to use it later
-	 * to avoid contention with the mutex.
-	 */
-	new_kmod_req = kzalloc_obj(*new_kmod_req);
-	if (!new_kmod_req)
-		return false;
+	if (!kmod_req)
+		return NULL;
 
-	refcount_set(&new_kmod_req->refcount, 1);
-	strscpy(new_kmod_req->name, module_name);
-	INIT_DELAYED_WORK(&new_kmod_req->delete_work, kmod_dup_request_delete);
-	init_completion(&new_kmod_req->first_req_done);
+	refcount_set(&kmod_req->refcount, 1);
+	strscpy(kmod_req->name, module_name);
+	INIT_DELAYED_WORK(&kmod_req->delete_work, kmod_dup_request_delete);
+	init_completion(&kmod_req->first_req_done);
+	return kmod_req;
+}
+
+bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
+{
+	struct kmod_dup_req *kmod_req;
+	int ret;
 
 	mutex_lock(&kmod_dup_mutex);
 
 	kmod_req = kmod_dup_request_lookup(module_name);
 	if (!kmod_req) {
+		struct kmod_dup_req *new_kmod_req;
+
 		/*
 		 * If the first request that came through for a module
 		 * was with request_module_nowait() we cannot wait for it
@@ -138,7 +141,6 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 		 * would benefit from duplicate detection.
 		 */
 		if (!wait) {
-			kfree(new_kmod_req);
 			pr_debug("New request_module_nowait() for %s -- cannot track duplicates for this request\n", module_name);
 			mutex_unlock(&kmod_dup_mutex);
 			return false;
@@ -149,6 +151,11 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 		 * keep tab on duplicates later.
 		 */
 		pr_debug("New request_module() for %s\n", module_name);
+		new_kmod_req = alloc_kmod_req(module_name);
+		if (!new_kmod_req) {
+			mutex_unlock(&kmod_dup_mutex);
+			return false;
+		}
 		list_add(&new_kmod_req->list, &dup_kmod_reqs);
 		mutex_unlock(&kmod_dup_mutex);
 		return false;
@@ -158,7 +165,6 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 	mutex_unlock(&kmod_dup_mutex);
 
 	/* We are dealing with a duplicate request now */
-	kfree(new_kmod_req);
 
 	/*
 	 * To fix these try to use try_then_request_module() instead as that

-- 
2.55.0

Re: [PATCH v3 3/6] module/dups: Avoid unnecessary kmod_dup_req allocations

[sashiko-bot]

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

Pre-existing issues:
- [High] A `request_module_nowait()` call can prematurely complete a concurrent `request_module(wait=true)` request, causing synchronous waiters to proceed before the module is actually loaded.
--

--- Patch [3]: [PATCH v3 3/6] module/dups: Avoid unnecessary kmod_dup_req allocations ---
commit c39253dbe61d24363b967a87af56bd7ef5c557ff
Author: Petr Pavlu <petr.pavlu@suse.com>

module/dups: Avoid unnecessary kmod_dup_req allocations

This commit defers the allocation of kmod_dup_req until it is actually
needed, instead of preallocating it before taking the mutex. It introduces
a helper alloc_kmod_req() to simplify the initialization process.

> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> --- a/kernel/module/dups.c
> +++ b/kernel/module/dups.c
[ ... ]
> @@ -138,7 +141,6 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
>  		 * If a prior request to this one came through with
>  		 * request_module() though, then a request_module_nowait()
>  		 * would benefit from duplicate detection.
>  		 */
>  		if (!wait) {
> -			kfree(new_kmod_req);
>  			pr_debug("New request_module_nowait() for %s -- cannot track duplicates for this request\n", module_name);
>  			mutex_unlock(&kmod_dup_mutex);
>  			return false;

[Severity: High]
This is a pre-existing issue, but does a request_module_nowait() call risk
prematurely completing a concurrent request_module() call that is waiting?

If Thread B calls request_module_nowait("foo"), it doesn't create a
tracking entry because wait is false. If Thread A subsequently calls
request_module("foo") with wait=true, it creates the tracking entry and
blocks for processing. If Thread C then calls request_module("foo") with
wait=true, it finds Thread A's entry and blocks waiting for it.

Because Thread B used nowait, its execution finishes quickly (waiting only
for the modprobe binary to exec) and returns 0. It then unconditionally
announces completion:

kernel/module/dups.c:kmod_dup_request_announce() {
        ...
        kmod_req = kmod_dup_request_lookup(module_name);
        if (!kmod_req || completion_done(&kmod_req->first_req_done)) {
                mutex_unlock(&kmod_dup_mutex);
                return;
        }

        kmod_req->dup_ret = ret;

        /* Inform all duplicate waiters to check the return value. */
        complete_all(&kmod_req->first_req_done);
        ...
}

Won't this find Thread A's tracking entry, assign dup_ret = 0, and wake up
Thread C prematurely while the module is still being loaded in the
background by Thread A?

If Thread C receives a success return value before the module is actually
fully loaded and initialized, could this cause drivers or subsystems to
attempt to use uninitialized module symbols or hardware features?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-module-stats-rcu-v3-0-2b0331106f00@suse.com?part=3

Re: [PATCH v3 3/6] module/dups: Avoid unnecessary kmod_dup_req allocations

[Petr Pavlu]

On 8/6/26 11:42 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] A `request_module_nowait()` call can prematurely complete a concurrent `request_module(wait=true)` request, causing synchronous waiters to proceed before the module is actually loaded.

This behavior is intentional.

[PATCH v3 4/6] module/dups: Use scope-based cleanup helpers

[Petr Pavlu]

Use scope-based cleanup helpers for kmod_dup_mutex and kmod_req to shorten
the code and to clarify where the lock is taken in
kmod_dup_request_exists_wait().

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/dups.c | 56 +++++++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index 276016051ad2..201a7f4c151f 100644
--- a/kernel/module/dups.c
+++ b/kernel/module/dups.c
@@ -63,6 +63,8 @@ static void put_kmod_req(struct kmod_dup_req *kmod_req)
 		kfree(kmod_req);
 }
 
+DEFINE_FREE(put_kmod_req, struct kmod_dup_req *, if (_T) put_kmod_req(_T))
+
 static struct kmod_dup_req *kmod_dup_request_lookup(char *module_name)
 {
 	struct kmod_dup_req *kmod_req;
@@ -97,9 +99,8 @@ static void kmod_dup_request_delete(struct work_struct *work)
 	 * kmod. The inneficies there are a call to modprobe and modprobe
 	 * just returning 0.
 	 */
-	mutex_lock(&kmod_dup_mutex);
-	list_del(&kmod_req->list);
-	mutex_unlock(&kmod_dup_mutex);
+	scoped_guard(mutex, &kmod_dup_mutex)
+		list_del(&kmod_req->list);
 
 	put_kmod_req(kmod_req);
 }
@@ -120,15 +121,18 @@ static struct kmod_dup_req *alloc_kmod_req(const char *module_name)
 
 bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 {
-	struct kmod_dup_req *kmod_req;
+	struct kmod_dup_req *kmod_req __free(put_kmod_req) = NULL;
 	int ret;
 
-	mutex_lock(&kmod_dup_mutex);
-
-	kmod_req = kmod_dup_request_lookup(module_name);
-	if (!kmod_req) {
+	scoped_guard(mutex, &kmod_dup_mutex) {
 		struct kmod_dup_req *new_kmod_req;
 
+		kmod_req = kmod_dup_request_lookup(module_name);
+		if (kmod_req) {
+			get_kmod_req(kmod_req);
+			break;
+		}
+
 		/*
 		 * If the first request that came through for a module
 		 * was with request_module_nowait() we cannot wait for it
@@ -142,7 +146,6 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 		 */
 		if (!wait) {
 			pr_debug("New request_module_nowait() for %s -- cannot track duplicates for this request\n", module_name);
-			mutex_unlock(&kmod_dup_mutex);
 			return false;
 		}
 
@@ -152,18 +155,12 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 		 */
 		pr_debug("New request_module() for %s\n", module_name);
 		new_kmod_req = alloc_kmod_req(module_name);
-		if (!new_kmod_req) {
-			mutex_unlock(&kmod_dup_mutex);
+		if (!new_kmod_req)
 			return false;
-		}
 		list_add(&new_kmod_req->list, &dup_kmod_reqs);
-		mutex_unlock(&kmod_dup_mutex);
 		return false;
 	}
 
-	get_kmod_req(kmod_req);
-	mutex_unlock(&kmod_dup_mutex);
-
 	/* We are dealing with a duplicate request now */
 
 	/*
@@ -190,7 +187,7 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 		 * calls bail out right away.
 		 */
 		*dup_ret = 0;
-		goto out;
+		return true;
 	}
 
 	/*
@@ -205,14 +202,11 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
 					TASK_KILLABLE);
 	if (ret) {
 		*dup_ret = ret;
-		goto out;
+		return true;
 	}
 
 	/* Now the duplicate request has the same exact return value as the first request */
 	*dup_ret = kmod_req->dup_ret;
-
-out:
-	put_kmod_req(kmod_req);
 	return true;
 }
 
@@ -220,26 +214,22 @@ void kmod_dup_request_announce(char *module_name, int ret)
 {
 	struct kmod_dup_req *kmod_req;
 
-	mutex_lock(&kmod_dup_mutex);
-
 	/*
 	 * Look for a kmod_dup_req previously added in
 	 * kmod_dup_request_exists_wait(). Note that a request_module_nowait()
 	 * without its own kmod_dup_req entry can announce a result of
 	 * a concurrent request_module() call.
 	 */
-	kmod_req = kmod_dup_request_lookup(module_name);
-	if (!kmod_req || completion_done(&kmod_req->first_req_done)) {
-		mutex_unlock(&kmod_dup_mutex);
-		return;
-	}
-
-	kmod_req->dup_ret = ret;
+	scoped_guard(mutex, &kmod_dup_mutex) {
+		kmod_req = kmod_dup_request_lookup(module_name);
+		if (!kmod_req || completion_done(&kmod_req->first_req_done))
+			return;
 
-	/* Inform all duplicate waiters to check the return value. */
-	complete_all(&kmod_req->first_req_done);
+		kmod_req->dup_ret = ret;
 
-	mutex_unlock(&kmod_dup_mutex);
+		/* Inform all duplicate waiters to check the return value. */
+		complete_all(&kmod_req->first_req_done);
+	}
 
 	/*
 	 * Now that we have allowed prior request_module() calls to go on

-- 
2.55.0

[PATCH v3 5/6] module/dups: Use strcmp() to compare module names

[Petr Pavlu]

Use strcmp() instead of strlen()+memcmp() to compare module names in
kmod_dup_request_lookup(), since all strings are NUL-terminated.

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/dups.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index 201a7f4c151f..fa3a4e623543 100644
--- a/kernel/module/dups.c
+++ b/kernel/module/dups.c
@@ -72,11 +72,9 @@ static struct kmod_dup_req *kmod_dup_request_lookup(char *module_name)
 	lockdep_assert_held(&kmod_dup_mutex);
 
 	list_for_each_entry(kmod_req, &dup_kmod_reqs, list) {
-		if (strlen(kmod_req->name) == strlen(module_name) &&
-		    !memcmp(kmod_req->name, module_name, strlen(module_name))) {
+		if (!strcmp(kmod_req->name, module_name))
 			return kmod_req;
-                }
-        }
+	}
 
 	return NULL;
 }

-- 
2.55.0

[PATCH v3 6/6] module/dups: Clean up includes

[Petr Pavlu]

The kernel/module/dups.c file relies on the following definitions and
associated functions:

* module_param() -> linux/moduleparam.h,
* DEFINE_MUTEX() -> linux/mutex.h,
* LIST_HEAD(), list_for_each_entry(), ... -> linux/list.h,
* refcount_t, refcount_inc(), ... -> linux/refcount.h,
* MODULE_NAME_LEN -> linux/module.h,
* completion, complete_all(), ... -> linux/completion.h,
* delayed_work, work_struct, ... -> linux/workqueue.h,
* lockdep_assert_held() -> linux/lockdep.h,
* strcmp(), memcpy() -> linux/string.h,
* container_of() -> linux/container_of.h,
* DEFINE_FREE(), __free(), scoped_guard() -> linux/cleanup.h,
* kzalloc_obj(), kfree() -> linux/slab.h,
* pr_debug(), pr_warn() -> linux/printk.h,
* WARN() -> linux/bug.h,
* TASK_KILLABLE -> linux/sched.h,
* HZ -> linux/param.h.

Update the file's include list accordingly.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/dups.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index fa3a4e623543..c9be9df6a65d 100644
--- a/kernel/module/dups.c
+++ b/kernel/module/dups.c
@@ -7,30 +7,22 @@
 
 #define pr_fmt(fmt)     "module: " fmt
 
+#include <linux/bug.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/container_of.h>
+#include <linux/list.h>
+#include <linux/lockdep.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/mutex.h>
+#include <linux/param.h>
+#include <linux/printk.h>
+#include <linux/refcount.h>
 #include <linux/sched.h>
-#include <linux/sched/task.h>
-#include <linux/binfmts.h>
-#include <linux/syscalls.h>
-#include <linux/unistd.h>
-#include <linux/kmod.h>
 #include <linux/slab.h>
-#include <linux/completion.h>
-#include <linux/cred.h>
-#include <linux/file.h>
+#include <linux/string.h>
 #include <linux/workqueue.h>
-#include <linux/security.h>
-#include <linux/mount.h>
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/resource.h>
-#include <linux/notifier.h>
-#include <linux/suspend.h>
-#include <linux/rwsem.h>
-#include <linux/ptrace.h>
-#include <linux/async.h>
-#include <linux/uaccess.h>
-#include <linux/refcount.h>
 
 #include "internal.h"
 

-- 
2.55.0

Re: [PATCH v3 0/6] module/dups: Fix a use-after-free and improve the code

[Petr Pavlu]

On 8/6/26 11:10 AM, Petr Pavlu wrote:
> Simplify how the kmod dups code informs duplicate requests about the
> result, fix a potential use-after-free when waiting on a duplicate request,
> and make several general improvements to the code.
> 
> Note that the use-after-free fix depends on the code simplification
> implemented in the first patch.

I've queued the series on modules-next for v7.3-rc1.

-- Petr