Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Mon, Feb 03 2025, Shyam Saini <shyamsaini@linux.microsoft.com> wrote:

> 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.
>
> Signed-off-by: Shyam Saini <shyamsaini@linux.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)

As others have said, this is way too big for an inline. Also, __init is
at best harmless (if it is indeed inlined into all callers), but if for
whatever reason the compiler decides to emit an OOL version, we
definitely do not want that in .init.text as we are now calling it from
non-init code.

> +{
> +	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);
> +

And while the BUG_ON() is borderline acceptable in the current,
only-used-during-init, state, that definitely must go away once the
function can be called from non-init code.

This is what I alluded to when I said that the function might need some
refactoring before module_add_driver can start using it.

I'd say that the BUG_ON can simply be removed and replaced by a return
NULL; an "impossible" error case that can already be hit by some of the
code below, so callers have to be prepared for it anyway. If the
allocation fails (during early boot or later), the allocator already
makes a lot of noise, so there's no reason to even do a WARN_ON, and
since it "only" affects certain /sys objects, it's probably better to
let the machine try to complete boot instead of killing it.

Also, I agree with the suggestion to drop/outdent the whole else{} branch by
changing the if() to do "return to_module_kobject(kobj);".

To summarize:

- refactor to do an early return

- drop BUG_ON, replace by return NULL.

- drop static and __init, perhaps change __init to __modinit (which is a
  pre-existing #define in params.c, so the function remains __init if
  !CONFIG_MODULES), add an appropriate declaration somewhere, and if you
  like, also do the rename

- make use of it from module_add_driver()

Rasmus