[PATCH v1 0/3] param: configurable /sys/module/*/paramaters

[PATCH v1 0/3] param: configurable /sys/module/*/paramaters

[David Decotigny]

These changes allow to compile kernels with sysfs but without
/sys/module/*/paramaters/. This should allow:
 - on small systems: no memory pressure caused by unneeded sysfs
   attributes.
 - on large systems: more modules could be converted to have their
   perm != 0 in module_param(): better for audit, debug, etc. For
   example, on my copy, 1658 module attributes have perm == 0,
   presumably to spare some memory, but it can be interesting to have
   access to those at runtime.

By default, /sys/module/*/paramaters/ is enabled, but can be disabled
in expert mode (CONFIG_SYSFS_MODULE_PARAM is not set).

David Decotigny (3):
  param: make destroy_params() private
  param: simple refactoring
  param: allow to selectively enable /sys/module/MOD/paramaters nodes

 fs/sysfs/Kconfig            |   15 ++++++-
 include/linux/moduleparam.h |   14 +-----
 kernel/module.c             |    9 ++++
 kernel/params.c             |  108 +++++++++++++++++++++----------------------
 4 files changed, 77 insertions(+), 69 deletions(-)

-- 
1.7.3.1

[PATCH v1 1/3] param: make destroy_params() private

[David Decotigny]

This moves params_destroy to module.c, the only place where it is
needed and called.

Tested:
  make defconfig all

Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 include/linux/moduleparam.h |   10 ----------
 kernel/module.c             |    9 +++++++++
 kernel/params.c             |    8 --------
 3 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index fffb10b..77d09f4 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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. */
diff --git a/kernel/module.c b/kernel/module.c
index 93342d9..529e965 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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)
 {
diff --git a/kernel/params.c b/kernel/params.c
index 8217889..75fd7bf 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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)
 {
-- 
1.7.3.1

[PATCH v1 2/3] param: simple refactoring

[David Decotigny]

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

Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 kernel/params.c |   93 ++++++++++++++++++++++++++----------------------------
 1 files changed, 45 insertions(+), 48 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 75fd7bf..b0e1668 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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(&param_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,
-- 
1.7.3.1

[PATCH v1 3/3] param: allow to selectively enable /sys/module/MOD/paramaters nodes

[David Decotigny]

This change defines CONFIG_SYSFS_MODULE_PARAM to prevent kernel module
parameters from being exposed to user. When unset, /sys/module/MOD is
populated as usual, except for the "parameters" sub-directory, which
is not created anymore.

Context: by default, when the module_param() attribute perm == 0, the
module attribute is not exposed to user in
/sys/module/*/parameters. Many module implementations use this
strategy, presumably to spare some memory. However, it can be
interesting to retrieve how kernel modules are configured at run-time
(debug, audit, etc.): it would be nice to see more modules have perm
!= 0 in order to expose their configuration pararemers to
user. Unfortunately, this doesn't play well with memory-constrained
systems that need sysfs but don't need this level of
introspection. This change allows to support both use cases.

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

Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 fs/sysfs/Kconfig            |   15 ++++++++++++++-
 include/linux/moduleparam.h |    4 ++--
 kernel/params.c             |    7 +++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/sysfs/Kconfig b/fs/sysfs/Kconfig
index 8c41fea..22ac50c 100644
--- a/fs/sysfs/Kconfig
+++ b/fs/sysfs/Kconfig
@@ -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
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 77d09f4..9c42200 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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);
diff --git a/kernel/params.c b/kernel/params.c
index b0e1668..7b2137a 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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;
 }
-- 
1.7.3.1

Re: [PATCH v1 3/3] param: allow to selectively enable /sys/module/MOD/paramaters nodes

[Rusty Russell]

Hi David,

        I'm having real trouble parsing your descriptions.  I found it
easier to read the patches, and that'd not good.

On Wed, 26 Oct 2011 19:22:27 -0700, David Decotigny <david.decotigny@google.com> wrote:
> This change defines CONFIG_SYSFS_MODULE_PARAM to prevent kernel module
> parameters from being exposed to user. When unset, /sys/module/MOD is
> populated as usual, except for the "parameters" sub-directory, which
> is not created anymore.

That's backwards.  CONFIG_SYSFS_MODULE_PARAM *enables*
/sys/module/<modname>/parameters, this implies it disables it.  Sure, it
allows it to be disabled without disabling all of CONFIG_SYSFS.

> Context: by default, when the module_param() attribute perm == 0, the
> module attribute is not exposed to user in
> /sys/module/*/parameters. Many module implementations use this
> strategy, presumably to spare some memory.

No, they use it because that was the default when I transferred them all
from the older module parameter system.  It was the safe choice.

> However, it can be
> interesting to retrieve how kernel modules are configured at run-time
> (debug, audit, etc.): it would be nice to see more modules have perm
> != 0 in order to expose their configuration pararemers to
> user.

But this patch doesn't address any of that.

> Unfortunately, this doesn't play well with memory-constrained
> systems that need sysfs but don't need this level of
> introspection. This change allows to support both use cases.

Do you have any statistics to support your assertion that this has any
significant effect on memory usage?

Your patches seem fine, but your descriptions are not straightforward!

Thanks,
Rusty.