lkml.org 
[lkml]   [2022]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH v5 1/2] module: update dependencies at try_module_get()
Date


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 <dan.j.williams@intel.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
>
> See [PATCH v5 0/2] at: https://lore.kernel.org/all/cover.1651348913.git.mchehab@kernel.org/
>
> 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)
> {
\
 
 \ /
  Last update: 2022-05-02 08:09    [W:0.073 / U:0.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site