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