| Message ID | 20250204052222.1611510-3-shyamsaini@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Properly handle module_kboject creation | expand |
| Context | Check | Description |
|---|---|---|
| mcgrof/vmtest-modules-next-VM_Test-1 | success | Logs for Run CI tests |
| mcgrof/vmtest-modules-next-VM_Test-0 | success | Logs for Run CI tests |
| mcgrof/vmtest-modules-next-VM_Test-4 | fail | Logs for setup / Setup kdevops environment |
| mcgrof/vmtest-modules-next-VM_Test-5 | fail | Logs for setup / Setup kdevops environment |
| mcgrof/vmtest-modules-next-PR | fail | PR summary |
| mcgrof/vmtest-modules-next-VM_Test-2 | fail | Logs for cleanup / Archive results and cleanup |
| mcgrof/vmtest-modules-next-VM_Test-3 | fail | Logs for cleanup / Archive results and cleanup |
> +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name) Crazily unreadable line. Either way this is not a function that should be inline in a header. Both due to sheer size, but also due to what it does and what it pulls in. > +{ > + struct module_kobject *mk; > + struct kobject *kobj; > + int err; > + > + kobj = kset_find_obj(module_kset, name); > + if (kobj) { > + mk = to_module_kobject(kobj); > + } else { And I know this is just moving the code, but an ealy return here would significanty clean up the function.. > + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL); > + BUG_ON(!mk); .. and a BUG_ON on allocation failure is a no-go.
Le 04/02/2025 à 06:22, Shyam Saini a écrit : > [Vous ne recevez pas souvent de courriers de shyamsaini@linux.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@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) > +{ > + 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 > >
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
Hi Everyone, On Wed, Feb 05, 2025 at 09:43:12AM +0100, Rasmus Villemoes wrote: > 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() I have addressed these feedback in v2, thank you for the reviews. Thanks, Shyam
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); + } 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; +} + #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)
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(-)