[PATCH 0/2] tracing: Fixes to event_triggers patches found by Fengguang's test bot

[PATCH 0/2] tracing: Fixes to event_triggers patches found by Fengguang's test bot

[Steven Rostedt]

Tom,

This is the changes I made to fix the reports that Fengguang's kbuild test bot
found. I folded in your change that fixes the bug with -ENODEV used in kernel.h.

Paul,

I've Cc'd you because the second patch is fixing up RCU notation. The filter
is protect by rcu_sched, and the first hunk of the patch is performed
within a rcu_read_lock_sched(), and the second hunk is done on the write
side. The tmp variable holds the old value, the value is updated, and
then the new value gets updated. I emailed you so that you can verify
that this all works.

Thanks!

-- Steve


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
ftrace/core

Head SHA1: 05741e383228a79eeec83644181255d2171f9f7c


Steven Rostedt (Red Hat) (2):
      tracing: Add generic tracing_lseek() function
      tracing: Fix rcu handling of event_trigger_data filter field

----
 include/linux/ftrace.h              |  2 --
 kernel/trace/ftrace.c               | 25 ++++++-------------------
 kernel/trace/trace.c                | 14 +++++++++-----
 kernel/trace/trace.h                |  4 +++-
 kernel/trace/trace_events_trigger.c | 10 ++++++----
 kernel/trace/trace_stack.c          |  2 +-
 6 files changed, 25 insertions(+), 32 deletions(-)

[PATCH 1/2] tracing: Add generic tracing_lseek() function

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-Add-generic-tracing_lseek-function.patch --]
[-- Type: text/plain, Size: 6484 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Trace event triggers added a lseek that uses the ftrace_filter_lseek()
function. Unfortunately, when function tracing is not configured in
that function is not defined and the kernel fails to build.

This is the second time that function was added to a file ops and
it broke the build due to requiring special config dependencies.

Make a generic tracing_lseek() that all the tracing utilities may
use.

Also, modify the old ftrace_filter_lseek() to return 0 instead of
1 on WRONLY. Not sure why it was a 1 as that does not make sense.

This also changes the old tracing_seek() to modify the file pos
pointer on WRONLY as well.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace.h              |  2 --
 kernel/trace/ftrace.c               | 25 ++++++-------------------
 kernel/trace/trace.c                | 14 +++++++++-----
 kernel/trace/trace.h                |  2 ++
 kernel/trace/trace_events_trigger.c |  2 +-
 kernel/trace/trace_stack.c          |  2 +-
 6 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 31ea4b4..f4233b1 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -570,8 +570,6 @@ static inline int
 ftrace_regex_release(struct inode *inode, struct file *file) { return -ENODEV; }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
-loff_t ftrace_filter_lseek(struct file *file, loff_t offset, int whence);
-
 /* totally disable ftrace - can not re-enable after this */
 void ftrace_kill(void);
 
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 0e9f9ea..531ffa6 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1082,19 +1082,6 @@ static __init void ftrace_profile_debugfs(struct dentry *d_tracer)
 
 static struct pid * const ftrace_swapper_pid = &init_struct_pid;
 
-loff_t
-ftrace_filter_lseek(struct file *file, loff_t offset, int whence)
-{
-	loff_t ret;
-
-	if (file->f_mode & FMODE_READ)
-		ret = seq_lseek(file, offset, whence);
-	else
-		file->f_pos = ret = 1;
-
-	return ret;
-}
-
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 #ifndef CONFIG_FTRACE_MCOUNT_RECORD
@@ -2739,7 +2726,7 @@ static void ftrace_filter_reset(struct ftrace_hash *hash)
  * routine, you can use ftrace_filter_write() for the write
  * routine if @flag has FTRACE_ITER_FILTER set, or
  * ftrace_notrace_write() if @flag has FTRACE_ITER_NOTRACE set.
- * ftrace_filter_lseek() should be used as the lseek routine, and
+ * tracing_lseek() should be used as the lseek routine, and
  * release must call ftrace_regex_release().
  */
 int
@@ -3767,7 +3754,7 @@ static const struct file_operations ftrace_filter_fops = {
 	.open = ftrace_filter_open,
 	.read = seq_read,
 	.write = ftrace_filter_write,
-	.llseek = ftrace_filter_lseek,
+	.llseek = tracing_lseek,
 	.release = ftrace_regex_release,
 };
 
@@ -3775,7 +3762,7 @@ static const struct file_operations ftrace_notrace_fops = {
 	.open = ftrace_notrace_open,
 	.read = seq_read,
 	.write = ftrace_notrace_write,
-	.llseek = ftrace_filter_lseek,
+	.llseek = tracing_lseek,
 	.release = ftrace_regex_release,
 };
 
@@ -4038,7 +4025,7 @@ static const struct file_operations ftrace_graph_fops = {
 	.open		= ftrace_graph_open,
 	.read		= seq_read,
 	.write		= ftrace_graph_write,
-	.llseek		= ftrace_filter_lseek,
+	.llseek		= tracing_lseek,
 	.release	= ftrace_graph_release,
 };
 
@@ -4046,7 +4033,7 @@ static const struct file_operations ftrace_graph_notrace_fops = {
 	.open		= ftrace_graph_notrace_open,
 	.read		= seq_read,
 	.write		= ftrace_graph_write,
-	.llseek		= ftrace_filter_lseek,
+	.llseek		= tracing_lseek,
 	.release	= ftrace_graph_release,
 };
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
@@ -4719,7 +4706,7 @@ static const struct file_operations ftrace_pid_fops = {
 	.open		= ftrace_pid_open,
 	.write		= ftrace_pid_write,
 	.read		= seq_read,
-	.llseek		= ftrace_filter_lseek,
+	.llseek		= tracing_lseek,
 	.release	= ftrace_pid_release,
 };
 
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 59bf5b5..e32a2f4 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3183,19 +3183,23 @@ tracing_write_stub(struct file *filp, const char __user *ubuf,
 	return count;
 }
 
-static loff_t tracing_seek(struct file *file, loff_t offset, int origin)
+loff_t tracing_lseek(struct file *file, loff_t offset, int whence)
 {
+	int ret;
+
 	if (file->f_mode & FMODE_READ)
-		return seq_lseek(file, offset, origin);
+		ret = seq_lseek(file, offset, whence);
 	else
-		return 0;
+		file->f_pos = ret = 0;
+
+	return ret;
 }
 
 static const struct file_operations tracing_fops = {
 	.open		= tracing_open,
 	.read		= seq_read,
 	.write		= tracing_write_stub,
-	.llseek		= tracing_seek,
+	.llseek		= tracing_lseek,
 	.release	= tracing_release,
 };
 
@@ -4940,7 +4944,7 @@ static const struct file_operations snapshot_fops = {
 	.open		= tracing_snapshot_open,
 	.read		= seq_read,
 	.write		= tracing_snapshot_write,
-	.llseek		= tracing_seek,
+	.llseek		= tracing_lseek,
 	.release	= tracing_snapshot_release,
 };
 
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 433bfc5..0380cab 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -588,6 +588,8 @@ void tracing_start_sched_switch_record(void);
 int register_tracer(struct tracer *type);
 int is_tracing_stopped(void);
 
+loff_t tracing_lseek(struct file *file, loff_t offset, int whence);
+
 extern cpumask_var_t __read_mostly tracing_buffer_mask;
 
 #define for_each_tracing_cpu(cpu)	\
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index f5b3f78..12ac8a5 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -281,7 +281,7 @@ const struct file_operations event_trigger_fops = {
 	.open = event_trigger_open,
 	.read = seq_read,
 	.write = event_trigger_write,
-	.llseek = ftrace_filter_lseek,
+	.llseek = tracing_lseek,
 	.release = event_trigger_release,
 };
 
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index b20428c..e6be585 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -382,7 +382,7 @@ static const struct file_operations stack_trace_filter_fops = {
 	.open = stack_trace_filter_open,
 	.read = seq_read,
 	.write = ftrace_filter_write,
-	.llseek = ftrace_filter_lseek,
+	.llseek = tracing_lseek,
 	.release = ftrace_regex_release,
 };
 
-- 
1.8.4.3

[PATCH 2/2] tracing: Fix rcu handling of event_trigger_data filter field

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-Fix-rcu-handling-of-event_trigger_data-filte.patch --]
[-- Type: text/plain, Size: 2288 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The filter field of the event_trigger_data structure is protected under
RCU sched locks. It was not annotated as such, and after doing so,
sparse pointed out several locations that required fix ups.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.h                | 2 +-
 kernel/trace/trace_events_trigger.c | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 0380cab..02b592f 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1057,7 +1057,7 @@ struct event_trigger_data {
 	int				ref;
 	struct event_trigger_ops	*ops;
 	struct event_command		*cmd_ops;
-	struct event_filter		*filter;
+	struct event_filter __rcu	*filter;
 	char				*filter_str;
 	void				*private_data;
 	struct list_head		list;
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index 12ac8a5..f6dd115 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -67,6 +67,7 @@ event_triggers_call(struct ftrace_event_file *file, void *rec)
 {
 	struct event_trigger_data *data;
 	enum event_trigger_type tt = ETT_NONE;
+	struct event_filter *filter;
 
 	if (list_empty(&file->triggers))
 		return tt;
@@ -76,7 +77,8 @@ event_triggers_call(struct ftrace_event_file *file, void *rec)
 			data->ops->func(data);
 			continue;
 		}
-		if (data->filter && !filter_match_preds(data->filter, rec))
+		filter = rcu_dereference(data->filter);
+		if (filter && !filter_match_preds(filter, rec))
 			continue;
 		if (data->cmd_ops->post_trigger) {
 			tt |= data->cmd_ops->trigger_type;
@@ -703,7 +705,7 @@ static int set_trigger_filter(char *filter_str,
 	if (ret)
 		goto out;
  assign:
-	tmp = data->filter;
+	tmp = rcu_access_pointer(data->filter);
 
 	rcu_assign_pointer(data->filter, filter);
 
@@ -719,7 +721,7 @@ static int set_trigger_filter(char *filter_str,
 	if (filter_str) {
 		data->filter_str = kstrdup(filter_str, GFP_KERNEL);
 		if (!data->filter_str) {
-			free_event_filter(data->filter);
+			free_event_filter(rcu_access_pointer(data->filter));
 			data->filter = NULL;
 			ret = -ENOMEM;
 		}
-- 
1.8.4.3

Re: [PATCH 2/2] tracing: Fix rcu handling of event_trigger_data filter field

[Paul E. McKenney]

On Sat, Dec 21, 2013 at 10:08:01PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> The filter field of the event_trigger_data structure is protected under
> RCU sched locks. It was not annotated as such, and after doing so,
> sparse pointed out several locations that required fix ups.
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Looks good to me!

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace.h                | 2 +-
>  kernel/trace/trace_events_trigger.c | 8 +++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 0380cab..02b592f 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1057,7 +1057,7 @@ struct event_trigger_data {
>  	int				ref;
>  	struct event_trigger_ops	*ops;
>  	struct event_command		*cmd_ops;
> -	struct event_filter		*filter;
> +	struct event_filter __rcu	*filter;
>  	char				*filter_str;
>  	void				*private_data;
>  	struct list_head		list;
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index 12ac8a5..f6dd115 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -67,6 +67,7 @@ event_triggers_call(struct ftrace_event_file *file, void *rec)
>  {
>  	struct event_trigger_data *data;
>  	enum event_trigger_type tt = ETT_NONE;
> +	struct event_filter *filter;
> 
>  	if (list_empty(&file->triggers))
>  		return tt;
> @@ -76,7 +77,8 @@ event_triggers_call(struct ftrace_event_file *file, void *rec)
>  			data->ops->func(data);
>  			continue;
>  		}
> -		if (data->filter && !filter_match_preds(data->filter, rec))
> +		filter = rcu_dereference(data->filter);
> +		if (filter && !filter_match_preds(filter, rec))
>  			continue;
>  		if (data->cmd_ops->post_trigger) {
>  			tt |= data->cmd_ops->trigger_type;
> @@ -703,7 +705,7 @@ static int set_trigger_filter(char *filter_str,
>  	if (ret)
>  		goto out;
>   assign:
> -	tmp = data->filter;
> +	tmp = rcu_access_pointer(data->filter);
> 
>  	rcu_assign_pointer(data->filter, filter);
> 
> @@ -719,7 +721,7 @@ static int set_trigger_filter(char *filter_str,
>  	if (filter_str) {
>  		data->filter_str = kstrdup(filter_str, GFP_KERNEL);
>  		if (!data->filter_str) {
> -			free_event_filter(data->filter);
> +			free_event_filter(rcu_access_pointer(data->filter));
>  			data->filter = NULL;
>  			ret = -ENOMEM;
>  		}
> -- 
> 1.8.4.3
> 
> 

Re: [PATCH 0/2] tracing: Fixes to event_triggers patches found by Fengguang's test bot

[Tom Zanussi]

Hi Steve,

On Sat, 2013-12-21 at 22:07 -0500, Steven Rostedt wrote:
> Tom,
> 
> This is the changes I made to fix the reports that Fengguang's kbuild test bot
> found. I folded in your change that fixes the bug with -ENODEV used in kernel.h.
> 

These look fine to me, and I didn't see any problems after running
through my normal event trigger testing with them applied.

So you can add my Acked-by and/or Tested-by..

Thanks for writing these improved versions!

Tom

> Paul,
> 
> I've Cc'd you because the second patch is fixing up RCU notation. The filter
> is protect by rcu_sched, and the first hunk of the patch is performed
> within a rcu_read_lock_sched(), and the second hunk is done on the write
> side. The tmp variable holds the old value, the value is updated, and
> then the new value gets updated. I emailed you so that you can verify
> that this all works.
> 
> Thanks!
> 
> -- Steve
> 
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> ftrace/core
> 
> Head SHA1: 05741e383228a79eeec83644181255d2171f9f7c
> 
> 
> Steven Rostedt (Red Hat) (2):
>       tracing: Add generic tracing_lseek() function
>       tracing: Fix rcu handling of event_trigger_data filter field
> 
> ----
>  include/linux/ftrace.h              |  2 --
>  kernel/trace/ftrace.c               | 25 ++++++-------------------
>  kernel/trace/trace.c                | 14 +++++++++-----
>  kernel/trace/trace.h                |  4 +++-
>  kernel/trace/trace_events_trigger.c | 10 ++++++----
>  kernel/trace/trace_stack.c          |  2 +-
>  6 files changed, 25 insertions(+), 32 deletions(-)

Re: [PATCH 0/2] tracing: Fixes to event_triggers patches found by Fengguang's test bot

[Steven Rostedt]

On Sun, 22 Dec 2013 21:13:02 -0600
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:

> Hi Steve,
> 
> On Sat, 2013-12-21 at 22:07 -0500, Steven Rostedt wrote:
> > Tom,
> > 
> > This is the changes I made to fix the reports that Fengguang's kbuild test bot
> > found. I folded in your change that fixes the bug with -ENODEV used in kernel.h.
> > 
> 
> These look fine to me, and I didn't see any problems after running
> through my normal event trigger testing with them applied.
> 
> So you can add my Acked-by and/or Tested-by..
> 
> Thanks for writing these improved versions!

Bah, I forgot to add these to the change logs before pushing to my
for-next branch. I hate to rebase, but I also would like these tags
too. 

Hmm, I may just rebase anyway. We still have a couple of weeks before
the merge window opens.

-- Steve