param: optional /sys/module/*/parameters
Subsystems (3)
v2 cover param: optional /sys/module/*/parameters
Hi,
Thank you Rusty for your review.
Here is the v2. I hope it is easier to understand. It also adds doc for the new Kconfig knob.
Description:
These patches turn /sys/module/*/parameters into an optional feature that can be enabled/disabled at compilation time. The goal is to encourage module developers to expose more of their parameters with fewer hesitations (eg. memory concerns, etc.), as the burden on small systems can now be avoided by disabling /sys/module/*/paramaters/ altogether at compile-time. On larger systems, having more module parameters exposed would help debugging/auditing of live systems.
Regards,
############################################
# Patch Set Summary:
David Decotigny (3):
param: make destroy_params() private
param: simple refactoring
param: make /sys/module/*/paramaters optional
Documentation/ABI/stable/sysfs-module | 3 + Documentation/DocBook/kgdb.tmpl | 3 +- fs/sysfs/Kconfig | 15 ++++- include/linux/moduleparam.h | 14 +---- kernel/module.c | 9 +++ kernel/params.c | 108 ++++++++++++++++----------------- 6 files changed, 82 insertions(+), 70 deletions(-)
Signature · 1 line
1.7.3.1
PATCH v2 1/3 param: make destroy_params() private
From: David Decotigny <[email protected]>
This moves params_destroy to module.c, the only place where it is needed and called.
Tested:
make defconfig all
--- include/linux/moduleparam.h | 10 ---------- kernel/module.c | 9 +++++++++ kernel/params.c | 8 -------- 3 files changed, 9 insertions(+), 18 deletions(-)
include/linux/moduleparam.h +0 -10
@@ -289,16 +289,6 @@ extern int parse_args(const char *name,unsigned num, int (*unknown)(char *param, char *val)); -/* Called by module remove. */ -#ifdef CONFIG_SYSFS -extern void destroy_params (const struct kernel_param *params,unsigned num); -#else -static inline void destroy_params (const struct kernel_param *params, -unsigned num) -{ -} -#endif /* !CONFIG_SYSFS */ -/* All the helper functions */ /* The macros to do compile-time type checking stolen from Jakub Jelinek, who IIRC came up with this idea for the 2.4 module init code. */
kernel/module.c +9 -0
@@ -1723,6 +1723,15 @@ void __weak module_arch_cleanup(struct module *mod) { } +static void destroy_params (const struct kernel_param *params,unsigned num) +{ +unsigned int i; + +for (i =0 ; i < num; i++) +if (params[i].ops->free) + params[i].ops->free (params[i].arg); +} +/* Free a module, remove from lists, etc. */ static void free_module (struct module *mod) {
kernel/params.c +0 -8
@@ -711,14 +711,6 @@ void module_param_sysfs_remove(struct module *mod) }#endif -void destroy_params (const struct kernel_param *params,unsigned num) -{ -unsigned int i; - -for (i =0 ; i < num; i++) -if (params[i].ops->free) - params[i].ops->free (params[i].arg); -}static struct module_kobject *__init locate_module_kobject (const char *name) {
Signature · 1 line
1.7.3.1
PATCH v2 2/3 param: simple refactoring
In reply to David DecotignyFrom: David Decotigny <[email protected]>
Moved locate_module_kobject() to limit the number of #ifdef/#endif blocks. Also moved __modinit macro definition at the top.
Tested:
make defconfig all
+ make all with # CONFIG_SYSFS is not set
--- kernel/params.c | 93 ++++++++++++++++++++++++++---------------------------- 1 files changed, 45 insertions(+), 48 deletions(-)
kernel/params.c +45 -48
@@ -31,6 +31,12 @@ #define DEBUGP(fmt, a...)#endif +#ifdef CONFIG_MODULES +#define __modinit +#else +#define __modinit __init +#endif +/* Protects all parameters, and incidentally kmalloced_param list. */ static DEFINE_MUTEX (param_lock);
@@ -515,6 +521,44 @@ struct module_param_attrs };#ifdef CONFIG_SYSFS +static struct module_kobject *__init locate_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); +printk (KERN_ERR +"Module '%s' failed add to sysfs, error number %d\n" , + name, err); +printk (KERN_ERR +"The system will be unstable now.\n" ); +return NULL ; + } + +/* So that we hold reference in both cases. */ +kobject_get (&mk->kobj); + } + +return mk; +} +#define to_param_attr (n) container_of(n, struct param_attribute, mattr) static ssize_t param_attr_show(struct module_attribute *mattr,
@@ -554,15 +598,7 @@ static ssize_t param_attr_store(struct module_attribute *mattr,return len;return err; } -#endif - -#ifdef CONFIG_MODULES -#define __modinit -#else -#define __modinit __init -#endif -#ifdef CONFIG_SYSFS void __kernel_param_lock (void ) {mutex_lock (¶m_lock);
@@ -709,46 +745,7 @@ void module_param_sysfs_remove(struct module *mod) free_module_param_attrs(&mod->mkobj); } } -#endif - - -static struct module_kobject *__init locate_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); -printk (KERN_ERR -"Module '%s' failed add to sysfs, error number %d\n" , - name, err); -printk (KERN_ERR -"The system will be unstable now.\n" ); -return NULL ; - } - -/* So that we hold reference in both cases. */ -kobject_get (&mk->kobj); - } - -return mk; -} +#endif /* CONFIG_MODULES */ static void __init kernel_add_sysfs_param(const char *name, struct kernel_param *kparam,
Signature · 1 line
1.7.3.1
PATCH v2 3/3 param: make /sys/module/*/paramaters optional
In reply to David DecotignyFrom: David Decotigny <[email protected]>
With this patch, we allow systems that don't want to pay the price for /sys/module/*/paramaters to be compiled without that feature. This abiltity can in turn encourage module developers to expose more of their parameters with fewer hesitations (eg. memory concerns, etc.); this is desirable to help debugging/auditing of live (larger) systems.
The new knob to control that is CONFIG_SYSFS_MODULE_PARAM available in Kconfig in expert mode: File systems / Pseudo filesystems / sysfs file system support / Module parameters in sysfs. It is enabled by default, keeping /sys/module/*/paramaters/ available as before.
As an illustration, on my copy I see 1658 module_param() macros with perm == 0: most of these could be exposed to user (perm != 0).
Tested:
- qemu + real x86_64 with defconfig customized:
CONFIG_EXPERT=y
CONFIG_SYSFS=y
# CONFIG_SYSFS_MODULE_PARAM is not set
- qemu + real x86_64 with defconfig
- make all with allyesconfig / allnoconfig / allmodconfig
--- Documentation/ABI/stable/sysfs-module | 3 +++ Documentation/DocBook/kgdb.tmpl | 3 ++- fs/sysfs/Kconfig | 15 ++++++++++++++- include/linux/moduleparam.h | 4 ++-- kernel/params.c | 7 +++++++ 5 files changed, 28 insertions(+), 4 deletions(-)
Documentation/ABI/stable/sysfs-module +3 -0
@@ -15,6 +15,9 @@ Description: documentation as to the contents of these parameters and what they accomplish. + This directory is available only when + CONFIG_SYSFS_MODULE_PARAM is set (default). + Note: The individual parameter names and values are not considered stable, only the fact that they will be placed in this location within sysfs. See the
Documentation/DocBook/kgdb.tmpl +2 -1
@@ -474,7 +474,8 @@ a kgdb I/O driver, kgdb will unregister all the kernel hook points. </para> <para> All kgdb I/O drivers can be reconfigured at run time, if - <symbol>CONFIG_SYSFS</symbol> and <symbol>CONFIG_MODULES</symbol> + <symbol>CONFIG_SYSFS</symbol>, <symbol>CONFIG_MODULES</symbol> and + <symbol>CONFIG_SYSFS_MODULE_PARAM</symbol> are enabled, by echo'ing a new config string to <constant>/sys/module/<driver>/parameter/<option></constant>. The driver can be unconfigured by passing an empty string. You cannot
fs/sysfs/Kconfig +14 -1
@@ -1,4 +1,4 @@ -config SYSFS +menuconfig SYSFS bool "sysfs file system support" if EXPERT default y help
@@ -21,3 +21,16 @@ config SYSFS example, "root=03:01" for /dev/hda1. Designers of embedded systems may wish to say N here to conserve space. + +if SYSFS + +config SYSFS_MODULE_PARAM + bool "Module parameters in sysfs" if EXPERT + default y + help + Allow to enable/disable the availability of kernel module + parameters in /sys/module/[module_name]/parameters. When + unset, this will conserve some memory space. If unsure, + say Y. + +endif # SYSFS
include/linux/moduleparam.h +2 -2
@@ -213,7 +213,7 @@ __check_old_set_param(int (*oldset)(const char *, struct kernel_param *))__kernel_param_unlock (); \ } while (0) -#ifdef CONFIG_SYSFS +#ifdef CONFIG_SYSFS_MODULE_PARAM extern void __kernel_param_lock (void );extern void __kernel_param_unlock (void );#else
@@ -400,7 +400,7 @@ extern int param_get_string(char *buffer, const struct kernel_param *kp);struct module ; -#if defined(CONFIG_SYSFS ) && defined(CONFIG_MODULES ) +#if defined(CONFIG_SYSFS_MODULE_PARAM ) && defined(CONFIG_MODULES )extern int module_param_sysfs_setup (struct module *mod,const struct kernel_param *kparam,unsigned int num_params);
kernel/params.c +7 -0
@@ -558,6 +558,9 @@ static struct module_kobject * __init locate_module_kobject(const char *name) return mk; } +#endif /* CONFIG_SYSFS */ + +#ifdef CONFIG_SYSFS_MODULE_PARAM #define to_param_attr (n) container_of(n, struct param_attribute, mattr)
@@ -805,7 +808,9 @@ static void __init param_sysfs_builtin(void) kernel_add_sysfs_param(modname, kp, name_len); } } +#endif /* CONFIG_SYSFS_MODULE_PARAM */ +#ifdef CONFIG_SYSFS ssize_t __modver_version_show (struct module_attribute *mattr,struct module_kobject *mk,char *buf) {
@@ -915,7 +920,9 @@ static int __init param_sysfs_init(void) module_sysfs_initialized =1 ;version_sysfs_builtin (); +#ifdef CONFIG_SYSFS_MODULE_PARAM param_sysfs_builtin (); +#endif /* CONFIG_SYSFS_MODULE_PARAM */ return 0 ; }
Signature · 1 line
1.7.3.1
Re: [PATCH v2 3/3] param: make /sys/module/*/paramaters optional
Quoted 4 lines · David Decotigny
From: David Decotigny <[email protected]>
With this patch, we allow systems that don't want to pay the price for
/sys/module/*/paramatersto be compiled without that feature.
The "price"? What kind of price is it? How much memory is really saved here?
greg k-h
Re: [PATCH v2 3/3] param: make /sys/module/*/paramaters optional
Thank you all for your feedback.
FYI, this patch was meant to be an answer to a negative feedback I received when I proposed changing a few module_param(... perm=S_IRUGO) in forcedeth driver. But I agree the "price to pay" is very low.
Should I drop this patch?
--
David Decotigny
Quoted 11 lines · Greg KH
Quoted 4 lines · David Decotigny
From: David Decotigny <[email protected]>
With this patch, we allow systems that don't want to pay the price for
/sys/module/*/paramatersto be compiled without that feature.The "price"? What kind of price is it? How much memory is really saved here?
greg k-h
Re: [PATCH v2 3/3] param: make /sys/module/*/paramaters optional
Quoted 5 lines · David Decotigny
Thank you all for your feedback.
FYI, this patch was meant to be an answer to a negative feedback I received when I proposed changing a few module_param(... perm=S_IRUGO) in forcedeth driver. But I agree the "price to pay" is very low.
Who gave you complaints about that? Really?
Quoted 1 line
Should I drop this patch?
Please do.
greg k-h
Re: [PATCH v2 3/3] param: make /sys/module/*/paramaters optional
In reply to David DecotignyQuoted 8 lines · David Decotigny
From: David Decotigny <[email protected]>
With this patch, we allow systems that don't want to pay the price for
/sys/module/*/paramatersto be compiled without that feature. This abiltity can in turn encourage module developers to expose more of their parameters with fewer hesitations (eg. memory concerns, etc.); this is desirable to help debugging/auditing of live (larger) systems.
Quoted 7 lines
The new knob to control that is
CONFIG_SYSFS_MODULE_PARAMavailable inKconfigin expert mode: File systems / Pseudo filesystems / sysfs file system support / Module parameters in sysfs. It is enabled by default, keeping/sys/module/*/paramaters/available as before.As an illustration, on my copy I see 1658
module_param()macros with perm == 0: most of these could be exposed to user (perm != 0).
Speaking as an embedded developer who works on very small systems that you can still debug, I am really curious what you actually save here?
For dynamic kgdb/kdb this patch is a death sentence. It would make us have to resurrect the procfs entries for the control point. There is no possible way to dynamically turn kgdb on and off at all if you remove the /sys/module/kgdboc/parameters/kgdboc entry, and this is certainly something where the typical use case is dynamic enablement.
Jason.
Re: [Kgdb-bugreport] [PATCH v2 3/3] param: make /sys/module/*/paramaters optional
Quoted 14 lines · Jason Wessel
Quoted 13 lines · David Decotigny
From: David Decotigny<[email protected]>
With this patch, we allow systems that don't want to pay the price for
/sys/module/*/paramatersto be compiled without that feature. This abiltity can in turn encourage module developers to expose more of their parameters with fewer hesitations (eg. memory concerns, etc.); this is desirable to help debugging/auditing of live (larger) systems.The new knob to control that is
CONFIG_SYSFS_MODULE_PARAMavailable inKconfigin expert mode: File systems / Pseudo filesystems / sysfs file system support / Module parameters in sysfs. It is enabled by default, keeping/sys/module/*/paramaters/available as before.
Quoted 5 lines
Quoted 2 lines
As an illustration, on my copy I see 1658
module_param()macros with perm == 0: most of these could be exposed to user (perm != 0).Speaking as an embedded developer who works on very small systems that you can still debug, I am really curious what you actually save here?
For dynamic kgdb/kdb this patch is a death sentence. It would make us have to resurrect the procfs entries for the control point. There is no possible way to dynamically turn kgdb on and off at all if you remove the
/sys/module/kgdboc/parameters/kgdbocentry, and this is certainly something where the typical use case is dynamic enablement.
I would also like to see the size numbers. However, to address Jason's concern, I think it's safe to assume that when someone is getting this aggressive with system size, they'll start leaving debug features on the floor (printk, kdb, etc.). What would be appropriate, IMHO, would be to add a config dependency for kgdb/kdb and NOT resurrect /proc items. There's work in progress in linux-tiny to tackle /proc configurability, and adding back /proc interfaces is likely to just have the issue resurface there.
-- Tim
Re: [Kgdb-bugreport] [PATCH v2 3/3] param: make /sys/module/*/paramaters optional
Quoted 11 lines · Tim Bird
Quoted 1 line · Jason Wessel
On 11/01/2011 06:50 PM, David Decotigny wrote:
I would also like to see the size numbers. However, to address Jason's concern, I think it's safe to assume that when someone is getting this aggressive with system size, they'll start leaving debug features on the floor (printk, kdb, etc.). What would be appropriate, IMHO, would be to add a config dependency for kgdb/kdb and NOT resurrect /proc items. There's work in progress in linux-tiny to tackle /proc configurability, and adding back /proc interfaces is likely to just have the issue resurface there.
-- Tim
This is already CONFIG_EXPERT; I don't think anyone really wants to turn this off.
Yes, I think the patch comes from a false premise: that the reason that people don't expost module params in sysfs is due to memory usage, and that providing a way to turn that off will alleviate their fears.
So, my advice to David: just change the perms on any and all module parameters you want. *Then* if someone complains about bloat, you can re-submit these patches and cc: me.
(BTW, don't like the first patch: static is nice, but keeping related code together is nicer).
Cheers,
Rusty.