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.willi...@intel.com>
> Reviewed-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Signed-off-by: Mauro Carvalho Chehab <mche...@kernel.org>
> ---
> 
> See [PATCH v5 0/2] at: 
> https://lore.kernel.org/all/cover.1651348913.git.mche...@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)
>   {

Reply via email to