| 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: <d0227f33-7b3f-426e-b731-b9a1f17020ed@csgroup.eu>
Date: Tue, 4 Feb 2025 10:28:32 +0100
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Shyam Saini <shyamsaini@...ux.microsoft.com>,
linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org
Cc: code@...icks.com, linux@...musvillemoes.dk, mcgrof@...nel.org,
frkaya@...ux.microsoft.com, vijayb@...ux.microsoft.com, petr.pavlu@...e.com,
linux@...ssschuh.net, samitolvanen@...gle.com, da.gomez@...sung.com,
gregkh@...uxfoundation.org, rafael@...nel.org, dakr@...nel.org
Subject: Re: [v1 2/3] include: move
lookup_or_create_module_kobject()/to_module* to module.h
Le 04/02/2025 à 06:22, Shyam Saini a écrit :
> [Vous ne recevez pas souvent de courriers de shyamsaini@...ux.microsoft.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>
> Move the following to module.h to allow common usages:
> - lookup_or_create_module_kobject()
> - to_module_attr
> - to_module_kobject
>
> This makes lookup_or_create_module_kobject() as inline.
Why an inline ? It looks quite big for an inline. Why not just make it
global ?
>
> Signed-off-by: Shyam Saini <shyamsaini@...ux.microsoft.com>
> ---
> include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
> kernel/params.c | 39 ---------------------------------------
> 2 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..ba5f5e6c3927 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
> #endif /* CONFIG_MODULES */
>
> #ifdef CONFIG_SYSFS
> +/* sysfs output in /sys/modules/XYZ/parameters/ */
> +#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> +#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> extern struct kset *module_kset;
> extern const struct kobj_type module_ktype;
> +
> +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
> +{
> + struct module_kobject *mk;
> + struct kobject *kobj;
> + int err;
> +
> + kobj = kset_find_obj(module_kset, name);
> + if (kobj) {
> + mk = to_module_kobject(kobj);
Would look cleaner without the else. Something like:
if (kobj)
return to_module_kobject(kobj);
mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
...
> + } else {
> + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> + BUG_ON(!mk);
Why a BUG_ON() ? Why not just return NULL and let the caller handle the
failure ?
> +
> + mk->mod = THIS_MODULE;
> + mk->kobj.kset = module_kset;
> + err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> + "%s", name);
> +#ifdef CONFIG_MODULES
This #ifdef is not needed, module_uevent is declared at all time in
module.h, IS_ENABLED(CONFIG_MODULES) should be enough.
> + if (!err)
> + err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
> +#endif
> + if (err) {
> + kobject_put(&mk->kobj);
> + pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
> + name, err);
> + return NULL;
> + }
> +
> + /* So that we hold reference in both cases. */
> + kobject_get(&mk->kobj);
> + }
> +
> + return mk;
> +}
> +
> #endif /* CONFIG_SYSFS */
>
> #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
> diff --git a/kernel/params.c b/kernel/params.c
> index 4b43baaf7c83..3c0798b79f76 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -537,10 +537,6 @@ const struct kernel_param_ops param_ops_string = {
> };
> EXPORT_SYMBOL(param_ops_string);
>
> -/* sysfs output in /sys/modules/XYZ/parameters/ */
> -#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> -#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> -
> struct param_attribute
> {
> struct module_attribute mattr;
> @@ -763,41 +759,6 @@ void destroy_params(const struct kernel_param *params, unsigned num)
> params[i].ops->free(params[i].arg);
> }
>
> -static struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
> -{
> - struct module_kobject *mk;
> - struct kobject *kobj;
> - int err;
> -
> - kobj = kset_find_obj(module_kset, name);
> - if (kobj) {
> - mk = to_module_kobject(kobj);
> - } else {
> - mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> - BUG_ON(!mk);
> -
> - mk->mod = THIS_MODULE;
> - mk->kobj.kset = module_kset;
> - err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> - "%s", name);
> -#ifdef CONFIG_MODULES
> - if (!err)
> - err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
> -#endif
> - if (err) {
> - kobject_put(&mk->kobj);
> - pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
> - name, err);
> - return NULL;
> - }
> -
> - /* So that we hold reference in both cases. */
> - kobject_get(&mk->kobj);
> - }
> -
> - return mk;
> -}
> -
> static void __init kernel_add_sysfs_param(const char *name,
> const struct kernel_param *kparam,
> unsigned int name_skip)
> --
> 2.34.1
>
>
Powered by blists - more mailing lists