Sometimes, device drivers are bound into each other via try_module_get(),
making such references invisible when looking at /proc/modules or lsmod.
Add a function to allow setting up module references for such
cases, and call it when try_module_get() is used.
Reviewed-by: Dan Williams <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
See [PATCH v5 0/2] at: https://lore.kernel.org/all/[email protected]/
include/linux/module.h | 8 ++++--
kernel/module/main.c | 65 +++++++++++++++++++++++++++++++++---------
2 files changed, 56 insertions(+), 17 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 46d4d5f2516e..3d9d38c426b4 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -620,12 +620,12 @@ 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 bool try_module_get_owner(struct module *module, struct module *this);
extern void module_put(struct module *module);
#else /*!CONFIG_MODULE_UNLOAD*/
-static inline bool try_module_get(struct module *module)
+static inline bool try_module_get_owner(struct module *module, struct module *this)
{
return !module || module_is_live(module);
}
@@ -740,7 +740,7 @@ static inline void __module_get(struct module *module)
{
}
-static inline bool try_module_get(struct module *module)
+static inline bool try_module_get_owner(struct module *module, struct module *this)
{
return true;
}
@@ -875,6 +875,8 @@ static inline bool module_sig_ok(struct module *module)
}
#endif /* CONFIG_MODULE_SIG */
+#define try_module_get(mod) try_module_get_owner(mod, THIS_MODULE)
+
int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
struct module *, unsigned long),
void *data);
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 05a42d8fcd7a..218c4308bb7a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -150,6 +150,24 @@ int unregister_module_notifier(struct notifier_block *nb)
}
EXPORT_SYMBOL(unregister_module_notifier);
+static bool __try_module_get(struct module *module)
+{
+ bool ret = true;
+
+ if (module) {
+ preempt_disable();
+ /* Note: here, we can fail to get a reference */
+ if (likely(module_is_live(module) &&
+ atomic_inc_not_zero(&module->refcnt) != 0))
+ trace_module_get(module, _RET_IP_);
+ else
+ ret = false;
+
+ preempt_enable();
+ }
+ return ret;
+}
+
/*
* We require a truly strong try_module_get(): 0 means success.
* Otherwise an error is returned due to ongoing or failed
@@ -160,7 +178,7 @@ 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))
+ if (__try_module_get(mod))
return 0;
else
return -ENOENT;
@@ -631,6 +649,33 @@ static int ref_module(struct module *a, struct module *b)
return 0;
}
+static int ref_module_dependency(struct module *mod, struct module *this)
+{
+ int ret;
+
+ if (!this || !this->name)
+ return -EINVAL;
+
+ if (mod == this)
+ return 0;
+
+ mutex_lock(&module_mutex);
+
+ ret = ref_module(this, mod);
+
+#ifdef CONFIG_MODULE_UNLOAD
+ if (ret)
+ goto ret;
+
+ ret = sysfs_create_link(mod->holders_dir,
+ &this->mkobj.kobj, this->name);
+#endif
+
+ret:
+ mutex_unlock(&module_mutex);
+ return ret;
+}
+
/* Clear the unload stuff of the module. */
static void module_unload_free(struct module *mod)
{
@@ -841,24 +886,16 @@ void __module_get(struct module *module)
}
EXPORT_SYMBOL(__module_get);
-bool try_module_get(struct module *module)
+bool try_module_get_owner(struct module *module, struct module *this)
{
- bool ret = true;
+ int ret = __try_module_get(module);
- if (module) {
- preempt_disable();
- /* Note: here, we can fail to get a reference */
- if (likely(module_is_live(module) &&
- atomic_inc_not_zero(&module->refcnt) != 0))
- trace_module_get(module, _RET_IP_);
- else
- ret = false;
+ if (ret)
+ ref_module_dependency(module, this);
- preempt_enable();
- }
return ret;
}
-EXPORT_SYMBOL(try_module_get);
+EXPORT_SYMBOL(try_module_get_owner);
void module_put(struct module *module)
{
--
2.35.1
Le 30/04/2022 à 22:04, Mauro Carvalho Chehab a écrit :
> Sometimes, device drivers are bound into each other via try_module_get(),
> making such references invisible when looking at /proc/modules or lsmod.
>
> Add a function to allow setting up module references for such
> cases, and call it when try_module_get() is used.
>
> Reviewed-by: Dan Williams <[email protected]>
> Reviewed-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
>
> See [PATCH v5 0/2] at: https://lore.kernel.org/all/[email protected]/
>
> include/linux/module.h | 8 ++++--
> kernel/module/main.c | 65 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 56 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 46d4d5f2516e..3d9d38c426b4 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -620,12 +620,12 @@ 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 bool try_module_get_owner(struct module *module, struct module *this);
You may want to remove that useless 'extern'.
'checkpatch --strict' will likely tell you to do so.
>
> extern void module_put(struct module *module);
>
> #else /*!CONFIG_MODULE_UNLOAD*/
> -static inline bool try_module_get(struct module *module)
> +static inline bool try_module_get_owner(struct module *module, struct module *this)
> {
> return !module || module_is_live(module);
> }
> @@ -740,7 +740,7 @@ static inline void __module_get(struct module *module)
> {
> }
>
> -static inline bool try_module_get(struct module *module)
> +static inline bool try_module_get_owner(struct module *module, struct module *this)
> {
> return true;
> }
> @@ -875,6 +875,8 @@ static inline bool module_sig_ok(struct module *module)
> }
> #endif /* CONFIG_MODULE_SIG */
>
> +#define try_module_get(mod) try_module_get_owner(mod, THIS_MODULE)
> +
> int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> struct module *, unsigned long),
> void *data);
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 05a42d8fcd7a..218c4308bb7a 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -150,6 +150,24 @@ int unregister_module_notifier(struct notifier_block *nb)
> }
> EXPORT_SYMBOL(unregister_module_notifier);
>
> +static bool __try_module_get(struct module *module)
> +{
> + bool ret = true;
> +
> + if (module) {
> + preempt_disable();
> + /* Note: here, we can fail to get a reference */
> + if (likely(module_is_live(module) &&
> + atomic_inc_not_zero(&module->refcnt) != 0))
> + trace_module_get(module, _RET_IP_);
> + else
> + ret = false;
> +
> + preempt_enable();
> + }
> + return ret;
> +}
> +
> /*
> * We require a truly strong try_module_get(): 0 means success.
> * Otherwise an error is returned due to ongoing or failed
> @@ -160,7 +178,7 @@ 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))
> + if (__try_module_get(mod))
> return 0;
> else
> return -ENOENT;
> @@ -631,6 +649,33 @@ static int ref_module(struct module *a, struct module *b)
> return 0;
> }
>
> +static int ref_module_dependency(struct module *mod, struct module *this)
What is 'this' ? Can we give it a more precise name ? Is it a child, a
parent, a owner, something else ?
> +{
> + int ret;
> +
> + if (!this || !this->name)
> + return -EINVAL;
> +
> + if (mod == this)
> + return 0;
> +
> + mutex_lock(&module_mutex);
> +
> + ret = ref_module(this, mod);
> +
> +#ifdef CONFIG_MODULE_UNLOAD
Looks like that #ifdef could be avoided and replaced by
IS_ENABLED(CONFIG_MODULE_UNLOAD)
Something like:
if (!IS_ENABLED(CONFIG_MODULE_UNLOAD) || ret)
goto ret;
> + if (ret)
> + goto ret;
> +
> + ret = sysfs_create_link(mod->holders_dir,
> + &this->mkobj.kobj, this->name);
> +#endif
> +
> +ret:
> + mutex_unlock(&module_mutex);
> + return ret;
> +}
> +
> /* Clear the unload stuff of the module. */
> static void module_unload_free(struct module *mod)
> {
> @@ -841,24 +886,16 @@ void __module_get(struct module *module)
> }
> EXPORT_SYMBOL(__module_get);
>
> -bool try_module_get(struct module *module)
> +bool try_module_get_owner(struct module *module, struct module *this)
Same here, what is 'this' exactly ?
> {
> - bool ret = true;
> + int ret = __try_module_get(module);
>
> - if (module) {
> - preempt_disable();
> - /* Note: here, we can fail to get a reference */
> - if (likely(module_is_live(module) &&
> - atomic_inc_not_zero(&module->refcnt) != 0))
> - trace_module_get(module, _RET_IP_);
> - else
> - ret = false;
> + if (ret)
> + ref_module_dependency(module, this);
>
> - preempt_enable();
> - }
> return ret;
> }
> -EXPORT_SYMBOL(try_module_get);
> +EXPORT_SYMBOL(try_module_get_owner);
>
> void module_put(struct module *module)
> {
Hi Mauro,
[...]
> +static int ref_module_dependency(struct module *mod, struct module *this)
> +{
> + int ret;
> +
> + if (!this || !this->name)
> + return -EINVAL;
> +
> + if (mod == this)
> + return 0;
> +
> + mutex_lock(&module_mutex);
> +
> + ret = ref_module(this, mod);
> +
> +#ifdef CONFIG_MODULE_UNLOAD
> + if (ret)
> + goto ret;
> +
> + ret = sysfs_create_link(mod->holders_dir,
> + &this->mkobj.kobj, this->name);
> +#endif
> +
> +ret:
> + mutex_unlock(&module_mutex);
> + return ret;
> +}
> +
> /* Clear the unload stuff of the module. */
> static void module_unload_free(struct module *mod)
> {
> @@ -841,24 +886,16 @@ void __module_get(struct module *module)
> }
> EXPORT_SYMBOL(__module_get);
>
> -bool try_module_get(struct module *module)
> +bool try_module_get_owner(struct module *module, struct module *this)
> {
> - bool ret = true;
> + int ret = __try_module_get(module);
>
> - if (module) {
> - preempt_disable();
> - /* Note: here, we can fail to get a reference */
> - if (likely(module_is_live(module) &&
> - atomic_inc_not_zero(&module->refcnt) != 0))
> - trace_module_get(module, _RET_IP_);
> - else
> - ret = false;
> + if (ret)
> + ref_module_dependency(module, this);
do we care about the return value here?
Andi
>
> - preempt_enable();
> - }
> return ret;
> }
> -EXPORT_SYMBOL(try_module_get);
> +EXPORT_SYMBOL(try_module_get_owner);
>
> void module_put(struct module *module)
> {
> --
> 2.35.1
Em Thu, 5 May 2022 23:35:29 +0200
Andi Shyti <[email protected]> escreveu:
> Hi Mauro,
>
> [...]
>
> > +static int ref_module_dependency(struct module *mod, struct module *this)
> > +{
> > + int ret;
> > +
> > + if (!this || !this->name)
> > + return -EINVAL;
> > +
> > + if (mod == this)
> > + return 0;
> > +
> > + mutex_lock(&module_mutex);
> > +
> > + ret = ref_module(this, mod);
> > +
> > +#ifdef CONFIG_MODULE_UNLOAD
> > + if (ret)
> > + goto ret;
> > +
> > + ret = sysfs_create_link(mod->holders_dir,
> > + &this->mkobj.kobj, this->name);
> > +#endif
> > +
> > +ret:
> > + mutex_unlock(&module_mutex);
> > + return ret;
> > +}
> > +
> > /* Clear the unload stuff of the module. */
> > static void module_unload_free(struct module *mod)
> > {
> > @@ -841,24 +886,16 @@ void __module_get(struct module *module)
> > }
> > EXPORT_SYMBOL(__module_get);
> >
> > -bool try_module_get(struct module *module)
> > +bool try_module_get_owner(struct module *module, struct module *this)
> > {
> > - bool ret = true;
> > + int ret = __try_module_get(module);
> >
> > - if (module) {
> > - preempt_disable();
> > - /* Note: here, we can fail to get a reference */
> > - if (likely(module_is_live(module) &&
> > - atomic_inc_not_zero(&module->refcnt) != 0))
> > - trace_module_get(module, _RET_IP_);
> > - else
> > - ret = false;
> > + if (ret)
> > + ref_module_dependency(module, this);
>
> do we care about the return value here?
I don't think it should care about the return value, as a failure to
create a sysfs node for the holder or to add it to the holders list
is not fatal: modules can still continue working without that.
Also, I opted to be conservative here: currently, not creating these
doesn't cause try_module_get() to fail. I'm not sure what would be the
impact if this starts to fail.
So, right now, I'm opting to just ignore the return value. Perhaps
in the future this could a warning (similarly to what sysfs create
link does).
Regards,
Mauro
>
> Andi
>
> >
> > - preempt_enable();
> > - }
> > return ret;
> > }
> > -EXPORT_SYMBOL(try_module_get);
> > +EXPORT_SYMBOL(try_module_get_owner);
> >
> > void module_put(struct module *module)
> > {
> > --
> > 2.35.1
Thanks,
Mauro