[PATCH v2 0/3] param: optional /sys/module/*/parameters

[PATCH v2 0/3] param: optional /sys/module/*/parameters

[David Decotigny]

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(-)

-- 
1.7.3.1

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

[David Decotigny]

From: David Decotigny <decot@google.com>

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 v2 2/3] param: simple refactoring

[David Decotigny]

From: David Decotigny <decot@google.com>

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 v2 3/3] param: make /sys/module/*/paramaters optional

[David Decotigny]

From: David Decotigny <decot@google.com>

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



Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 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(-)

diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module
index 75be431..a733ad8 100644
--- a/Documentation/ABI/stable/sysfs-module
+++ b/Documentation/ABI/stable/sysfs-module
@@ -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
diff --git a/Documentation/DocBook/kgdb.tmpl b/Documentation/DocBook/kgdb.tmpl
index d71b57f..68a1d23 100644
--- a/Documentation/DocBook/kgdb.tmpl
+++ b/Documentation/DocBook/kgdb.tmpl
@@ -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/&lt;driver&gt;/parameter/&lt;option&gt;</constant>.
    The driver can be unconfigured by passing an empty string.  You cannot
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 v2 3/3] param: make /sys/module/*/paramaters optional

[Greg KH]

On Tue, Nov 01, 2011 at 04:50:35PM -0700, David Decotigny wrote:
> From: David Decotigny <decot@google.com>
> 
> With this patch, we allow systems that don't want to pay the price for
> /sys/module/*/paramaters to 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

[Jason Wessel]

On 11/01/2011 06:50 PM, David Decotigny wrote:
> From: David Decotigny <decot@google.com>
>
> 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).

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

[Tim Bird]

On 11/2/2011 6:30 AM, Jason Wessel wrote:
> On 11/01/2011 06:50 PM, David Decotigny wrote:
>> From: David Decotigny<decot@google.com>
>>
>> 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).
> 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.
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

[Rusty Russell]

On Wed, 2 Nov 2011 09:24:35 -0700, Tim Bird <tim.bird@am.sony.com> wrote:
> On 11/2/2011 6:30 AM, Jason Wessel wrote:
> > 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.

Re: [PATCH v2 3/3] param: make /sys/module/*/paramaters optional

[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.

Should I drop this patch?

--
David Decotigny



On Tue, Nov 1, 2011 at 5:24 PM, Greg KH <gregkh@suse.de> wrote:
> On Tue, Nov 01, 2011 at 04:50:35PM -0700, David Decotigny wrote:
>> From: David Decotigny <decot@google.com>
>>
>> With this patch, we allow systems that don't want to pay the price for
>> /sys/module/*/paramaters to 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

[Greg KH]

On Wed, Nov 02, 2011 at 04:02:24PM -0700, David Decotigny wrote:
> 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?

> Should I drop this patch?

Please do.

greg k-h