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

[PATCH v2 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 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: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260722-module-stats-rcu-ff0dfc986be6

[PATCH v2 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 1d720a5311ba..b063bc423aa8 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;
 
 	memcpy(new_kmod_req->name, module_name, strlen(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.54.0

[PATCH v2 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")
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 b063bc423aa8..99661236490a 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);
 	memcpy(new_kmod_req->name, module_name, strlen(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.54.0

[PATCH v2 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.

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 99661236490a..0f6bb05b2d81 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);
-	memcpy(new_kmod_req->name, module_name, strlen(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);
+	memcpy(kmod_req->name, module_name, strlen(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.54.0

[PATCH v2 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 0f6bb05b2d81..56d659f049c5 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.54.0

[PATCH v2 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.

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 56d659f049c5..80e1638ac34a 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.54.0

[PATCH v2 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 80e1638ac34a..872db6159b48 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.54.0

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

[sashiko-bot]

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

    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/20260723-module-stats-rcu-v2-0-bd582261fae3@suse.com?part=1