Thread (24 messages) 24 messages, 4 authors, 2022-01-25

Re: [PATCH bpf-next v6 01/11] kernel: Implement try_module_get_live

From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2022-01-25 18:51:20
Also in: bpf, linux-modules, lkml, netfilter-devel

On Sun, Jan 02, 2022 at 09:51:05PM +0530, Kumar Kartikeya Dwivedi wrote:
quoted hunk ↗ jump to hunk
Refactor shared functionality between strong_try_module_get and
try_module_get into a common helper, and expose try_module_get_live
that returns a bool similar to try_module_get.

It will be used in the next patch for btf_try_get_module, to eliminate a
race between module __init function invocation and module_put from BPF
side.

Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-modules@vger.kernel.org
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/module.h | 26 +++++++++++++++++++-------
 kernel/module.c        | 20 ++++++++------------
 2 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index c9f1200b2312..eb83aaeaa76e 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -608,17 +608,17 @@ void symbol_put_addr(void *addr);
 /* Sometimes we know we already have a refcount, and it's easier not
    to handle the error case (which only happens with rmmod --wait). */
 extern void __module_get(struct module *module);
-
-/* This is the Right Way to get a module: if it fails, it's being removed,
- * so pretend it's not there. */
-extern bool try_module_get(struct module *module);
-
+extern int __try_module_get(struct module *module, bool strong);
 extern void module_put(struct module *module);
 
 #else /*!CONFIG_MODULE_UNLOAD*/
-static inline bool try_module_get(struct module *module)
+static inline int __try_module_get(struct module *module, bool strong)
 {
-	return !module || module_is_live(module);
+	if (module && !module_is_live(module))
+		return -ENOENT;
+	if (strong && module && module->state == MODULE_STATE_COMING)
+		return -EBUSY;
+	return 0;
 }
The bool return is clear here before on try_module_get().
quoted hunk ↗ jump to hunk
 static inline void module_put(struct module *module)
 {
@@ -631,6 +631,18 @@ static inline void __module_get(struct module *module)
 
 #endif /* CONFIG_MODULE_UNLOAD */
 
+/* This is the Right Way to get a module: if it fails, it's being removed,
+ * so pretend it's not there. */
+static inline bool try_module_get(struct module *module)
+{
+	return !__try_module_get(module, false);
Now you're making it negate an int return... 
quoted hunk ↗ jump to hunk
+}
+/* Only take reference for modules which have fully initialized */
+static inline bool try_module_get_live(struct module *module)
+{
+	return !__try_module_get(module, true);
+}
+
 /* This is a #define so the string doesn't get put in every .o file */
 #define module_name(mod)			\
 ({						\
diff --git a/kernel/module.c b/kernel/module.c
index 84a9141a5e15..a9bb0a5576c8 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -318,12 +318,7 @@ EXPORT_SYMBOL(unregister_module_notifier);
 static inline int strong_try_module_get(struct module *mod)
 {
 	BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED);
-	if (mod && mod->state == MODULE_STATE_COMING)
-		return -EBUSY;
-	if (try_module_get(mod))
-		return 0;
-	else
-		return -ENOENT;
Before this change, this check had no disabled preemption
prior to the first branch, now we are having it moved with
preemption disabled. That's an OK change, but it is a
small functional change.

Because of these two things NACK on this patch for now.
Please split the patch up if you intend to make a new
functional change. And this patch should be easy to read,
this is not.

  Luis
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help