module: Allow parameters without arguments

Currently the params.c code allows only two "set" functions to have no arguments. If a parameter does not have an argument, then it looks at the set function and tests if it is either param_set_bool() or param_set_bint(). If it is not one of these functions, then it fails the loading of the module.

But there may be module parameters that have different set functions and still allow no arguments. But unless each of these cases adds their function to the if statement, it wont be allowed to have no arguments. This method gets rather messing and does not scale.

Instead, introduce a flags field to the kernel_param_ops, where if the flag KERNEL_PARAM_FL_NOARG is set, the parameter will not fail if it does not contain an argument. It will be expected that the corresponding set function can handle a NULL pointer as "val".

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/moduleparam.h |   13 ++++++++++++-
 kernel/params.c             |    6 ++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

include/linux/moduleparam.h +12 -1

index 27d9da3..c3eb102 100644
@@ -36,7 +36,18 @@ static const char __UNIQUE_ID(name)[]					  \
 
 struct kernel_param;
 
+/*
+ * Flags available for kernel_param_ops
+ *
+ * NOARG - the parameter allows for no argument (foo instead of foo=1)
+ */
+enum {
+	KERNEL_PARAM_FL_NOARG = (1 << 0)
+};
+
 struct kernel_param_ops {
+	/* How the ops should behave */
+	unsigned int flags;
 	/* Returns 0, or -errno.  arg is in kp->arg. */
 	int (*set)(const char *val, const struct kernel_param *kp);
 	/* Returns length written or -errno.  Buffer is 4k (ie. be short!) */

@@ -187,7 +198,7 @@ struct kparam_array
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, set, get, arg, perm)			\
 	static struct kernel_param_ops __param_ops_##name =		\
-		 { (void *)set, (void *)get };				\
+		{ 0, (void *)set, (void *)get };			\
 	__module_param_call(MODULE_PARAM_PREFIX,			\
 			    name, &__param_ops_##name, arg,		\
 			    (perm) + sizeof(__check_old_set_param(set))*0, -1)

kernel/params.c +4 -2

index 440e65d..27a0af9 100644
@@ -103,8 +103,8 @@ static int parse_one(char *param,
 			    || params[i].level > max_level)
 				return 0;
 			/* No one handled NULL, so do it here. */
-			if (!val && params[i].ops->set != param_set_bool
-			    && params[i].ops->set != param_set_bint)
+			if (!val &&
+			    !(params[i].ops->flags & KERNEL_PARAM_FL_NOARG))
 				return -EINVAL;
 			pr_debug("handling %s with %p\n", param,
 				params[i].ops->set);

@@ -320,6 +320,7 @@ int param_get_bool(char *buffer, const struct kernel_param *kp)
 EXPORT_SYMBOL(param_get_bool);
 
 struct kernel_param_ops param_ops_bool = {
+	.flags = KERNEL_PARAM_FL_NOARG,
 	.set = param_set_bool,
 	.get = param_get_bool,
 };

@@ -370,6 +371,7 @@ int param_set_bint(const char *val, const struct kernel_param *kp)
 EXPORT_SYMBOL(param_set_bint);
 
 struct kernel_param_ops param_ops_bint = {
+	.flags = KERNEL_PARAM_FL_NOARG,
 	.set = param_set_bint,
 	.get = param_get_int,
 };

Signature · 1 line

1.7.10.4

Rusty,

I'm looking at porting my "enable tracepoints in module load" patches and one of the comments you gave me (long ago) was to not have:

trace_foo=1

but to just have:

trace_foo

as a parameter name. I went and implemented this but discovered that the functions that allow no arguments are hard coded in the params.c file.

I changed this to allow other "set" functions to be given no arguments, and even noticed that a few already exist in the kernel. So I'm sending you this patch set that implements a modification to the parameter parsing to allow other kernel_param_ops to not bother with arguments passed in.

What do you think?

-- Steve

Steven Rostedt (1):
tracing: Enable tracepoints via module parameters

Steven Rostedt (Red Hat) (3):
module: Add flag to allow mod params to have no arguments
module: Add NOARG flag for ops with param_set_bool_enable_only() set function module/lsm: Have apparmor module parameters work with no args

----

 include/linux/ftrace_event.h |    4 +++
 include/linux/moduleparam.h  |   13 +++++++-
 include/trace/ftrace.h       |   23 ++++++++++++--
 kernel/module.c              |    1 +
 kernel/params.c              |    6 ++--
 kernel/trace/trace_events.c  |   71 ++++++++++++++++++++++++++++++++++++++++++
 security/apparmor/lsm.c      |    2 ++
 7 files changed, 115 insertions(+), 5 deletions(-)

---------------------------

include/linux/ftrace_event.h +4 -0

index 120d57a..0395182 100644
@@ -164,6 +164,8 @@ void tracing_record_cmdline(struct task_struct *tsk);
 
 struct event_filter;
 
+extern struct kernel_param_ops ftrace_mod_ops;
+
 enum trace_reg {
 	TRACE_REG_REGISTER,
 	TRACE_REG_UNREGISTER,

@@ -202,6 +204,7 @@ enum {
 	TRACE_EVENT_FL_NO_SET_FILTER_BIT,
 	TRACE_EVENT_FL_IGNORE_ENABLE_BIT,
 	TRACE_EVENT_FL_WAS_ENABLED_BIT,
+	TRACE_EVENT_FL_MOD_ENABLE_BIT,
 };
 
 /*

@@ -220,6 +223,7 @@ enum {
 	TRACE_EVENT_FL_NO_SET_FILTER	= (1 << TRACE_EVENT_FL_NO_SET_FILTER_BIT),
 	TRACE_EVENT_FL_IGNORE_ENABLE	= (1 << TRACE_EVENT_FL_IGNORE_ENABLE_BIT),
 	TRACE_EVENT_FL_WAS_ENABLED	= (1 << TRACE_EVENT_FL_WAS_ENABLED_BIT),
+	TRACE_EVENT_FL_MOD_ENABLE	= (1 << TRACE_EVENT_FL_MOD_ENABLE_BIT),
 };
 
 struct ftrace_event_call {

include/linux/moduleparam.h +12 -1

index 27d9da3..c3eb102 100644
@@ -36,7 +36,18 @@ static const char __UNIQUE_ID(name)[]					  \
 
 struct kernel_param;
 
+/*
+ * Flags available for kernel_param_ops
+ *
+ * NOARG - the parameter allows for no argument (foo instead of foo=1)
+ */
+enum {
+	KERNEL_PARAM_FL_NOARG = (1 << 0)
+};
+
 struct kernel_param_ops {
+	/* How the ops should behave */
+	unsigned int flags;
 	/* Returns 0, or -errno.  arg is in kp->arg. */
 	int (*set)(const char *val, const struct kernel_param *kp);
 	/* Returns length written or -errno.  Buffer is 4k (ie. be short!) */

@@ -187,7 +198,7 @@ struct kparam_array
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, set, get, arg, perm)			\
 	static struct kernel_param_ops __param_ops_##name =		\
-		 { (void *)set, (void *)get };				\
+		{ 0, (void *)set, (void *)get };			\
 	__module_param_call(MODULE_PARAM_PREFIX,			\
 			    name, &__param_ops_##name, arg,		\
 			    (perm) + sizeof(__check_old_set_param(set))*0, -1)

include/trace/ftrace.h +21 -2

index 41a6643..d6029ed 100644
@@ -17,6 +17,7 @@
  */
 
 #include <linux/ftrace_event.h>
+#include <linux/module.h>
 
 /*
  * DECLARE_EVENT_CLASS can be used to add a generic function

@@ -577,6 +578,22 @@ static inline void ftrace_test_probe_##call(void)			\
 #undef __get_dynamic_array
 #undef __get_str
 
+/*
+ * Add ftrace trace points in modules to be set by module
+ * parameters. This adds "trace_##call" as a module parameter.
+ * The user could enable trace points on module load with:
+ *  trace_##call=1 as a module parameter.
+ */
+#undef ftrace_mod_params
+#ifdef MODULE
+#define ftrace_mod_params(call)				\
+	module_param_cb(trace_##call, &ftrace_mod_ops,	\
+			&event_##call, 0644);		\
+	MODULE_INFO(tracepoint, #call)
+#else
+#define ftrace_mod_params(call)
+#endif
+
 #undef TP_printk
 #define TP_printk(fmt, args...) "\"" fmt "\", "  __stringify(args)
 

@@ -604,7 +621,8 @@ static struct ftrace_event_call __used event_##call = {			\
 	.print_fmt		= print_fmt_##template,			\
 };									\
 static struct ftrace_event_call __used					\
-__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
+__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call;\
+ftrace_mod_params(call)
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, call, proto, args, print)		\

@@ -618,7 +636,8 @@ static struct ftrace_event_call __used event_##call = {			\
 	.print_fmt		= print_fmt_##call,			\
 };									\
 static struct ftrace_event_call __used					\
-__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
+__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call; \
+ftrace_mod_params(call)
 
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
 

kernel/module.c +1 -0

index 2069158..4eb26b6 100644
@@ -136,6 +136,7 @@ static int param_set_bool_enable_only(const char *val,
 }
 
 static const struct kernel_param_ops param_ops_bool_enable_only = {
+	.flags = KERNEL_PARAM_FL_NOARG,
 	.set = param_set_bool_enable_only,
 	.get = param_get_bool,
 };

kernel/params.c +4 -2

index 440e65d..27a0af9 100644
@@ -103,8 +103,8 @@ static int parse_one(char *param,
 			    || params[i].level > max_level)
 				return 0;
 			/* No one handled NULL, so do it here. */
-			if (!val && params[i].ops->set != param_set_bool
-			    && params[i].ops->set != param_set_bint)
+			if (!val &&
+			    !(params[i].ops->flags & KERNEL_PARAM_FL_NOARG))
 				return -EINVAL;
 			pr_debug("handling %s with %p\n", param,
 				params[i].ops->set);

@@ -320,6 +320,7 @@ int param_get_bool(char *buffer, const struct kernel_param *kp)
 EXPORT_SYMBOL(param_get_bool);
 
 struct kernel_param_ops param_ops_bool = {
+	.flags = KERNEL_PARAM_FL_NOARG,
 	.set = param_set_bool,
 	.get = param_get_bool,
 };

@@ -370,6 +371,7 @@ int param_set_bint(const char *val, const struct kernel_param *kp)
 EXPORT_SYMBOL(param_set_bint);
 
 struct kernel_param_ops param_ops_bint = {
+	.flags = KERNEL_PARAM_FL_NOARG,
 	.set = param_set_bint,
 	.get = param_get_int,
 };

kernel/trace/trace_events.c +71 -0

index 29a7ebc..5bd3f51 100644
@@ -1839,10 +1839,32 @@ trace_create_file_ops(struct module *mod)
 	return file_ops;
 }
 
+static void
+enable_event_on_trace_array(struct trace_array *tr,
+			    struct ftrace_event_call *call)
+{
+	struct ftrace_event_file *file;
+	int ret;
+
+	call->flags &= ~TRACE_EVENT_FL_MOD_ENABLE;
+
+	/* find event on top trace array */
+	list_for_each_entry(file, &tr->events, list) {
+		if (file->event_call == call) {
+			ret = ftrace_event_enable_disable(file, 1);
+			if (ret < 0)
+				pr_warning("unable to enable tracepoint %s",
+					   call->name);
+			return;
+		}
+	}
+}
+
 static void trace_module_add_events(struct module *mod)
 {
 	struct ftrace_module_file_ops *file_ops = NULL;
 	struct ftrace_event_call **call, **start, **end;
+	struct trace_array *tr = NULL;
 
 	start = mod->trace_events;
 	end = mod->trace_events + mod->num_trace_events;

@@ -1857,6 +1879,12 @@ static void trace_module_add_events(struct module *mod)
 	for_each_event(call, start, end) {
 		__register_event(*call, mod);
 		__add_event_to_tracers(*call, file_ops);
+		/* If the module tracepoint parameter was set */
+		if ((*call)->flags & TRACE_EVENT_FL_MOD_ENABLE) {
+			if (!tr)
+				tr = top_trace_array();
+			enable_event_on_trace_array(tr, *call);
+		}
 	}
 }
 

@@ -2569,6 +2597,49 @@ early_initcall(event_trace_memsetup);
 core_initcall(event_trace_enable);
 fs_initcall(event_trace_init);
 
+/* Allow modules to load with enabled trace points */
+int ftrace_mod_param_set(const char *val, const struct kernel_param *kp)
+{
+	struct ftrace_event_call *call = kp->arg;
+
+	/* This is set like param_set_bool() */
+
+	/* No equals means "set"... */
+	if (!val)
+		val = "1";
+
+	/* One of =[yYnN01] */
+	switch (val[0]) {
+	case 'y': case 'Y': case '1':
+		break;
+	case 'n': case 'N': case '0':
+		/* Do nothing */
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	/* Set flag to tell ftrace to enable this event on init */
+	call->flags = TRACE_EVENT_FL_MOD_ENABLE;
+
+	return 0;
+}
+
+int ftrace_mod_param_get(char *buffer, const struct kernel_param *kp)
+{
+	struct ftrace_event_call *call = kp->arg;
+
+	return sprintf(buffer, "%d",
+		       !!(call->flags & TRACE_EVENT_FL_MOD_ENABLE));
+}
+
+struct kernel_param_ops ftrace_mod_ops = {
+	.flags = KERNEL_PARAM_FL_NOARG,
+	.set = ftrace_mod_param_set,
+	.get = ftrace_mod_param_get,
+};
+EXPORT_SYMBOL(ftrace_mod_ops);
+
 #ifdef CONFIG_FTRACE_STARTUP_TEST
 
 static DEFINE_SPINLOCK(test_spinlock);

security/apparmor/lsm.c +2 -0

index 2e2a0dd..e3a704c 100644
@@ -666,6 +666,7 @@ static int param_set_aabool(const char *val, const struct kernel_param *kp);
 static int param_get_aabool(char *buffer, const struct kernel_param *kp);
 #define param_check_aabool param_check_bool
 static struct kernel_param_ops param_ops_aabool = {
+	.flags = KERNEL_PARAM_FL_NOARG,
 	.set = param_set_aabool,
 	.get = param_get_aabool
 };

@@ -682,6 +683,7 @@ static int param_set_aalockpolicy(const char *val, const struct kernel_param *kp
 static int param_get_aalockpolicy(char *buffer, const struct kernel_param *kp);
 #define param_check_aalockpolicy param_check_bool
 static struct kernel_param_ops param_ops_aalockpolicy = {
+	.flags = KERNEL_PARAM_FL_NOARG,
 	.set = param_set_aalockpolicy,
 	.get = param_get_aalockpolicy
 };

Rusty,

I'm looking at porting my "enable tracepoints in module load" patches and one of the comments you gave me (long ago) was to not have:

trace_foo=1

but to just have:

trace_foo

as a parameter name. I went and implemented this but discovered that the functions that allow no arguments are hard coded in the params.c file.

I changed this to allow other "set" functions to be given no arguments, and even noticed that a few already exist in the kernel. So I'm sending you this patch set that implements a modification to the parameter parsing to allow other kernel_param_ops to not bother with arguments passed in.

What do you think?

so in kcmdline we would have modulename.param instead of modulename.param=1?

I guess we need to update kmod then, because currently we ignore and treat this case as a wrong token. From a quick look, allowing it in kmod would be as simple as removing a condition check.

Lucas De Marchi

Quoted 7 lines · On Tue, 13 Aug 2013 20:34:58 -0300 Lucas De Marchi <[email protected]>…

so in kcmdline we would have modulename.param instead of modulename.param=1?

I guess we need to update kmod then, because currently we ignore and treat this case as a wrong token. From a quick look, allowing it in kmod would be as simple as removing a condition check.

Lucas De Marchi

Note, both will still work. And it didn't change much. Today, anything that uses "module_param()" with bool type (a quick git grep shows 570 users), already do not require a value.

Randomly looking at one... drivers/input/mouse/synaptics_i2c.c, you can just do:

insmod synaptics_i2c.ko no_filter

no need to add a "=1" to that.

But anything else will still require a value. I just want to allow other parameters that act like a boolean to not require one.

Quoted 23 lines · Steven Rostedt

Quoted 7 lines · On Tue, 13 Aug 2013 20:34:58 -0300 Lucas De Marchi <[email protected]>…

so in kcmdline we would have modulename.param instead of modulename.param=1?

I guess we need to update kmod then, because currently we ignore and treat this case as a wrong token. From a quick look, allowing it in kmod would be as simple as removing a condition check.

Lucas De Marchi

Note, both will still work. And it didn't change much. Today, anything that uses "module_param()" with bool type (a quick git grep shows 570 users), already do not require a value.

Randomly looking at one... drivers/input/mouse/synaptics_i2c.c, you can just do:

insmod synaptics_i2c.ko no_filter

no need to add a "=1" to that.

Quoted 2 lines

But anything else will still require a value. I just want to allow other parameters that act like a boolean to not require one.

true... but currently "modprobe synaptics_i2c" doesn't get the parameter correctly from kernel command line if it doesn't have a value. And I agree this not something that changed but rather a bug in kmod waiting to be fixed.