| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d728cc1b-d34f-77a5-a1fb-81cc117a3620@csgroup.eu>
Date: Mon, 2 May 2022 06:08:32 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Mauro Carvalho Chehab <mchehab@...nel.org>,
Luis Chamberlain <mcgrof@...nel.org>
CC: Daniel Vetter <daniel@...ll.ch>, David Airlie <airlied@...ux.ie>,
Greg KH <gregkh@...uxfoundation.org>,
Jaroslav Kysela <perex@...ex.cz>,
Kai Vehmanen <kai.vehmanen@...el.com>,
Lucas De Marchi <lucas.demarchi@...el.com>,
Pierre-Louis Bossart <pierre-louis.bossart@...el.com>,
Takashi Iwai <tiwai@...e.com>,
"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
"mauro.chehab@...ux.intel.com" <mauro.chehab@...ux.intel.com>,
Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH v5 1/2] module: update dependencies at try_module_get()
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@...el.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@...nel.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)
> {
Powered by blists - more mailing lists