linux-kernel - Re: [v2 2/3] kernel: refactor and globalize lookup_or_create_module

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <307ec0eb-df8e-4031-9bfa-227b1cd2fcb7@t-8ch.de>
Date: Fri, 7 Feb 2025 08:35:28 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Shyam Saini <shyamsaini@...ux.microsoft.com>
Cc: linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org, 
	code@...icks.com, linux@...musvillemoes.dk, christophe.leroy@...roup.eu, 
	hch@...radead.org, mcgrof@...nel.org, frkaya@...ux.microsoft.com, 
	vijayb@...ux.microsoft.com, petr.pavlu@...e.com, samitolvanen@...gle.com, 
	da.gomez@...sung.com, gregkh@...uxfoundation.org, rafael@...nel.org, dakr@...nel.org
Subject: Re: [v2 2/3] kernel: refactor and globalize
 lookup_or_create_module_kobject()

On 2025-02-06 21:45:37-0800, Shyam Saini wrote:
> lookup_or_create_module_kobject() is static and marked as __init,
> this is not ideal for global usage.

FYI missing "PATCH" in patch subject.

> Fix this limitation by refactoring and declaring this as global:
>  - Refactor it by removing BUG_ON() and 'if else' construct by returning
>    early

This does look like an unrelated change, could be in its own patch.

>  - Remove static and __init markers from the function and add its
>    declaration in module.h
>  - Mark this function as "__modinit". To facilitate this, move the
>    __modinit macro construct to module.h
> 
> Suggested-by: Rasmus Villemoes <linux@...musvillemoes.dk>
> Signed-off-by: Shyam Saini <shyamsaini@...ux.microsoft.com>
> ---
>  include/linux/module.h |  8 +++++++
>  kernel/params.c        | 48 ++++++++++++++++++------------------------
>  2 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..57d09b4e4385 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -162,6 +162,14 @@ extern void cleanup_module(void);
>  #define __INITRODATA_OR_MODULE __INITRODATA
>  #endif /*CONFIG_MODULES*/
>  
> +#ifdef CONFIG_MODULES
> +#define __modinit
> +#else
> +#define __modinit __init
> +#endif
> +
> +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name);

__init / __modinit is not necessary on the declaration.
You can remove it here and keep the #define private.

> +
>  /* Generic info of form tag = "info" */
>  #define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info)
>  
> diff --git a/kernel/params.c b/kernel/params.c
> index 4b43baaf7c83..5d16696b1daa 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -595,12 +595,6 @@ static ssize_t param_attr_store(const struct module_attribute *mattr,
>  }
>  #endif
>  
> -#ifdef CONFIG_MODULES
> -#define __modinit
> -#else
> -#define __modinit __init
> -#endif
> -
>  #ifdef CONFIG_SYSFS
>  void kernel_param_lock(struct module *mod)
>  {
> @@ -763,38 +757,38 @@ 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 __modinit * 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,
> +	if (kobj)
> +		return to_module_kobject(kobj);
> +
> +	mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> +	if (!mk)
> +		return NULL;
> +
> +	mk->mod = THIS_MODULE;
> +	mk->kobj.kset = module_kset;
> +	err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
>  					   "%s", name);
>  #ifdef CONFIG_MODULES

As you are cleaning this up anyways: The #ifdef above should become

if (IS_ENABLED(CONFIG_MODULES))

> -		if (!err)
> -			err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
> +	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);
> +	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;
>  }
>  
> -- 
> 2.34.1
>