[PATCH 0/3] module: Allow parameters without arguments

[PATCH 0/3] module: Allow parameters without arguments

[Steven Rostedt]

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(-)
---------------------------
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 120d57a..0395182 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -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 {
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 27d9da3..c3eb102 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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)
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 41a6643..d6029ed 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -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)
 
diff --git a/kernel/module.c b/kernel/module.c
index 2069158..4eb26b6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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,
 };
diff --git a/kernel/params.c b/kernel/params.c
index 440e65d..27a0af9 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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,
 };
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 29a7ebc..5bd3f51 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -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);
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 2e2a0dd..e3a704c 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -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
 };

[PATCH 1/3] [PATCH 1/3] module: Add flag to allow mod params to have no arguments

[Steven Rostedt]

[-- Attachment #1: 0001-module-Add-flag-to-allow-mod-params-to-have-no-argum.patch --]
[-- Type: text/plain, Size: 3215 bytes --]

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 <rostedt@goodmis.org>
---
 include/linux/moduleparam.h |   13 ++++++++++++-
 kernel/params.c             |    6 ++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 27d9da3..c3eb102 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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)
diff --git a/kernel/params.c b/kernel/params.c
index 440e65d..27a0af9 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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,
 };
-- 
1.7.10.4

[PATCH 2/3] [PATCH 2/3] module: Add NOARG flag for ops with param_set_bool_enable_only() set function

[Steven Rostedt]

[-- Attachment #1: 0002-module-Add-NOARG-flag-for-ops-with-param_set_bool_en.patch --]
[-- Type: text/plain, Size: 695 bytes --]

The ops that uses param_set_bool_enable_only() as its set function can
easily handle being used without an argument. There's no reason to
fail the loading of the module if it does not have one.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/module.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/module.c b/kernel/module.c
index 2069158..4eb26b6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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,
 };
-- 
1.7.10.4

[PATCH 3/3] [PATCH 3/3] module/lsm: Have apparmor module parameters work with no args

[Steven Rostedt]

[-- Attachment #1: 0003-module-lsm-Have-apparmor-module-parameters-work-with.patch --]
[-- Type: text/plain, Size: 1388 bytes --]

The apparmor module parameters for param_ops_aabool and
param_ops_aalockpolicy are both based off of the param_ops_bool,
and can handle a NULL value passed in as val. Have it enable the
new KERNEL_PARAM_FL_NOARGS flag to allow the parameters to be set
without having to state "=y" or "=1".

Cc: John Johansen <john.johansen@canonical.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 security/apparmor/lsm.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 2e2a0dd..e3a704c 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -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
 };
-- 
1.7.10.4

Re: [PATCH 0/3] module: Allow parameters without arguments

[Steven Rostedt]

On Tue, 13 Aug 2013 17:02:28 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

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

OK, this is what I get for using my scripts along with manually sending
out patches via quilt. I only wanted to send out the three patches
below, but then used my scripts to make this header. The above patch
commit (along with the complete change set below) was not what I
intended on sending :-p

-- Steve



> 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(-)
> ---------------------------
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 120d57a..0395182 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -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 {
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 27d9da3..c3eb102 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -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)
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 41a6643..d6029ed 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -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)
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index 2069158..4eb26b6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -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,
>  };
> diff --git a/kernel/params.c b/kernel/params.c
> index 440e65d..27a0af9 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -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,
>  };
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 29a7ebc..5bd3f51 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -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);
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 2e2a0dd..e3a704c 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -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
>  };

Re: [PATCH 0/3] module: Allow parameters without arguments

[Lucas De Marchi]

On Tue, Aug 13, 2013 at 6:02 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 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

Re: [PATCH 0/3] module: Allow parameters without arguments

[Steven Rostedt]

On Tue, 13 Aug 2013 20:34:58 -0300
Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:

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

-- Steve

Re: [PATCH 0/3] module: Allow parameters without arguments

[Lucas De Marchi]

On Tue, Aug 13, 2013 at 9:17 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 13 Aug 2013 20:34:58 -0300
> Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
>
>
>> 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.

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.

Lucas De Marchi

Re: [PATCH 0/3] module: Allow parameters without arguments

[Lucas De Marchi]

On Tue, Aug 13, 2013 at 10:00 PM, Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:
> On Tue, Aug 13, 2013 at 9:17 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Tue, 13 Aug 2013 20:34:58 -0300
>> Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
>>
>>
>>> 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.
>
> 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.

And it's fixed now with a proper test added.

thanks

Lucas De Marchi

Re: [PATCH 1/3] [PATCH 1/3] module: Add flag to allow mod params to have no arguments

[Rusty Russell]

Steven Rostedt <rostedt@goodmis.org> writes:
> 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".

Good idea.  This hack was introduced because people wrote their own
param parsers which didn't expect NULL, leading to oopsen.

A flag is a better solution.

Applied all three.

Thanks,
Rusty.


>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  include/linux/moduleparam.h |   13 ++++++++++++-
>  kernel/params.c             |    6 ++++--
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 27d9da3..c3eb102 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -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)
> diff --git a/kernel/params.c b/kernel/params.c
> index 440e65d..27a0af9 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -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,
>  };
> -- 
> 1.7.10.4