[PATCH ftrace/core 0/3] ftrace: Fix three small bugs

[PATCH ftrace/core 0/3] ftrace: Fix three small bugs

[Yoshihiro YUNOMAE]

Hi Steven,

I found three small bugs for current ftrace/core, so I fixed those.
Would you apply these patches?

Thanks!

---

Yoshihiro YUNOMAE (3):
      trace/event: Return error if ftrace_trace_arrays is empty list
      trace/kprobes: Avoid self tests if tracing is disabled on boot up
      trace: Fix memory leak when new instance creation failed


 kernel/trace/trace.c        |   23 ++++++++++++++---------
 kernel/trace/trace.h        |    3 +++
 kernel/trace/trace_events.c |   13 +++++++++++++
 kernel/trace/trace_kprobe.c |    3 +++
 4 files changed, 33 insertions(+), 9 deletions(-)

-- 
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com

[PATCH ftrace/core 1/3] trace/event: Return error if ftrace_trace_arrays is empty list

[Yoshihiro YUNOMAE]

ftrace_trace_arrays links global_trace.list. However, global_trace is not added
to ftrace_trace_arrays if trace_alloc_buffers() failed. As the result,
ftrace_trace_arrays becomes empty list. If ftrace_trace_arrays is empty list,
current top_trace_array() returns invalid pointer. As the result, the kernel
can induce memory corruption or panic.

Current implementation does not check whether ftrace_trace_arrays is empty
list or not. So, in this patch, if ftrace_trace_arrays is empty list,
top_trace_array() returns NULL. Moreover, this patch makes all functions calling
top_trace_array() handle it appropriately.

Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace.h        |    3 +++
 kernel/trace/trace_events.c |   13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 217207a..9e82551 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -252,6 +252,9 @@ static inline struct trace_array *top_trace_array(void)
 {
 	struct trace_array *tr;
 
+	if (list_empty(ftrace_trace_arrays.prev))
+		return NULL;
+
 	tr = list_entry(ftrace_trace_arrays.prev,
 			typeof(*tr), list);
 	WARN_ON(!(tr->flags & TRACE_ARRAY_FL_GLOBAL));
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 3ddfd8f..1349870 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -574,6 +574,9 @@ int trace_set_clr_event(const char *system, const char *event, int set)
 {
 	struct trace_array *tr = top_trace_array();
 
+	if (!tr)
+		return -ENODEV;
+
 	return __ftrace_set_clr_event(tr, NULL, system, event, set);
 }
 EXPORT_SYMBOL_GPL(trace_set_clr_event);
@@ -2065,6 +2068,9 @@ event_enable_func(struct ftrace_hash *hash,
 	bool enable;
 	int ret;
 
+	if (!tr)
+		return -ENODEV;
+
 	/* hash funcs only work with set_ftrace_filter */
 	if (!enabled || !param)
 		return -EINVAL;
@@ -2396,6 +2402,9 @@ static __init int event_trace_enable(void)
 	char *token;
 	int ret;
 
+	if (!tr)
+		return -ENODEV;
+
 	for_each_event(iter, __start_ftrace_events, __stop_ftrace_events) {
 
 		call = *iter;
@@ -2442,6 +2451,8 @@ static __init int event_trace_init(void)
 	int ret;
 
 	tr = top_trace_array();
+	if (!tr)
+		return -ENODEV;
 
 	d_tracer = tracing_init_dentry();
 	if (!d_tracer)
@@ -2535,6 +2546,8 @@ static __init void event_trace_self_tests(void)
 	int ret;
 
 	tr = top_trace_array();
+	if (!tr)
+		return -ENODEV;
 
 	pr_info("Running tests on trace events:\n");
 

Re: [PATCH ftrace/core 1/3] trace/event: Return error if ftrace_trace_arrays is empty list

[Steven Rostedt]

On Fri, 06 Jun 2014 07:35:17 +0900
Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com> wrote:

> ftrace_trace_arrays links global_trace.list. However, global_trace is not added
> to ftrace_trace_arrays if trace_alloc_buffers() failed. As the result,
> ftrace_trace_arrays becomes empty list. If ftrace_trace_arrays is empty list,
> current top_trace_array() returns invalid pointer. As the result, the kernel
> can induce memory corruption or panic.
> 
> Current implementation does not check whether ftrace_trace_arrays is empty
> list or not. So, in this patch, if ftrace_trace_arrays is empty list,
> top_trace_array() returns NULL. Moreover, this patch makes all functions calling
> top_trace_array() handle it appropriately.

As I'm still working on some more patches for 3.16, I can add this.
It's not that critical, because if global_array fails to allocate on
boot up, lots of other things may also break.

-- Steve

> 
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> ---

[PATCH ftrace/core] tracing: Remove return value in event_trace_self_tests() when top_trace_array() returns NULL

[Yoshihiro YUNOMAE]

Remove return value in event_trace_self_tests() when top_trace_array() returns
NULL because event_trace_self_tests() is void type.

Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace_events.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 1349870..f99e0b3 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2547,7 +2547,7 @@ static __init void event_trace_self_tests(void)
 
 	tr = top_trace_array();
 	if (!tr)
-		return -ENODEV;
+		return;
 
 	pr_info("Running tests on trace events:\n");
 

Re: [PATCH ftrace/core] tracing: Remove return value in event_trace_self_tests() when top_trace_array() returns NULL

[Yoshihiro YUNOMAE]

Hi Steven,

Thank you for applying my patches.
I received build warning report from kbuild test bot,
so I fixed it. Would you apply this patch?

Thank you,
Yoshihiro YUNOMAE

(2014/06/06 15:28), Yoshihiro YUNOMAE wrote:
> Remove return value in event_trace_self_tests() when top_trace_array() returns
> NULL because event_trace_self_tests() is void type.
>
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>   kernel/trace/trace_events.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 1349870..f99e0b3 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2547,7 +2547,7 @@ static __init void event_trace_self_tests(void)
>
>   	tr = top_trace_array();
>   	if (!tr)
> -		return -ENODEV;
> +		return;
>
>   	pr_info("Running tests on trace events:\n");
>
>
>

-- 
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com

Re: [PATCH ftrace/core] tracing: Remove return value in event_trace_self_tests() when top_trace_array() returns NULL

[Steven Rostedt]

On Fri, 06 Jun 2014 15:31:15 +0900
Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com> wrote:

> Hi Steven,
> 
> Thank you for applying my patches.
> I received build warning report from kbuild test bot,
> so I fixed it. Would you apply this patch?
>

My tests also caught it. I pushed to my ftrace/core branch which is
allowed to rebase (I push only to trigger Wu's bots). I'll just fold
that fix into the original patch.

Thanks,

-- Steve

[PATCH ftrace/core 2/3] trace/kprobes: Avoid self tests if tracing is disabled on boot up

[Yoshihiro YUNOMAE]

If tracing is disabled on boot up, the kernel should not execute self tests.
In this patch, the kernel checks whether tracing is disabled or not
before executing self tests.

Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace_kprobe.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 903ae28..ef2fba1 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1377,6 +1377,9 @@ static __init int kprobe_trace_self_tests_init(void)
 	struct trace_kprobe *tk;
 	struct ftrace_event_file *file;
 
+	if (tracing_is_disabled())
+		return -ENODEV;
+
 	target = kprobe_trace_selftest_target;
 
 	pr_info("Testing kprobe tracing: ");

Re: [PATCH ftrace/core 2/3] trace/kprobes: Avoid self tests if tracing is disabled on boot up

[Masami Hiramatsu]

(2014/06/06 7:35), Yoshihiro YUNOMAE wrote:
> If tracing is disabled on boot up, the kernel should not execute self tests.
> In this patch, the kernel checks whether tracing is disabled or not
> before executing self tests.

Looks good to me, thanks! :)

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

> 
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/trace/trace_kprobe.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 903ae28..ef2fba1 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1377,6 +1377,9 @@ static __init int kprobe_trace_self_tests_init(void)
>  	struct trace_kprobe *tk;
>  	struct ftrace_event_file *file;
>  
> +	if (tracing_is_disabled())
> +		return -ENODEV;
> +
>  	target = kprobe_trace_selftest_target;
>  
>  	pr_info("Testing kprobe tracing: ");
> 
> 


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH ftrace/core 3/3] trace: Fix memory leak when new instance creation failed

[Yoshihiro YUNOMAE]

Current new_instance_create() implements just two fail paths for four
allocation operations. So, it can induce memory leak if new instance
creation failed. This patch fixes it by defining all fail paths and
freeing allocated memories appropriately.

Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 473eb68..bbd86d2 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6277,7 +6277,7 @@ static int new_instance_create(const char *name)
 		goto out_free_tr;
 
 	if (!alloc_cpumask_var(&tr->tracing_cpumask, GFP_KERNEL))
-		goto out_free_tr;
+		goto out_free_tr_name;
 
 	cpumask_copy(tr->tracing_cpumask, cpu_all_mask);
 
@@ -6291,16 +6291,16 @@ static int new_instance_create(const char *name)
 	INIT_LIST_HEAD(&tr->events);
 
 	if (allocate_trace_buffers(tr, trace_buf_size) < 0)
-		goto out_free_tr;
+		goto out_free_cpumask_var;
 
 	tr->dir = debugfs_create_dir(name, trace_instance_dir);
 	if (!tr->dir)
-		goto out_free_tr;
+		goto out_free_trace_buffers;
 
 	ret = event_trace_add_tracer(tr->dir, tr);
 	if (ret) {
 		debugfs_remove_recursive(tr->dir);
-		goto out_free_tr;
+		goto out_free_trace_buffers;
 	}
 
 	init_tracer_debugfs(tr, tr->dir);
@@ -6311,18 +6311,23 @@ static int new_instance_create(const char *name)
 
 	return 0;
 
- out_free_tr:
-	if (tr->trace_buffer.buffer)
-		ring_buffer_free(tr->trace_buffer.buffer);
+ out_free_trace_buffers:
+	ring_buffer_free(tr->trace_buffer.buffer);
+	free_percpu(tr->trace_buffer.data);
+#ifdef CONFIG_TRACER_MAX_TRACE
+	ring_buffer_free(tr->max_buffer.buffer);
+	free_percpu(tr->max_buffer.data);
+#endif
+ out_free_cpumask_var:
 	free_cpumask_var(tr->tracing_cpumask);
+ out_free_tr_name:
 	kfree(tr->name);
+ out_free_tr:
 	kfree(tr);
-
  out_unlock:
 	mutex_unlock(&trace_types_lock);
 
 	return ret;
-
 }
 
 static int instance_delete(const char *name)

Re: [PATCH ftrace/core 3/3] trace: Fix memory leak when new instance creation failed

[Steven Rostedt]

On Fri, 06 Jun 2014 07:35:22 +0900
Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com> wrote:

> Current new_instance_create() implements just two fail paths for four
> allocation operations. So, it can induce memory leak if new instance
> creation failed. This patch fixes it by defining all fail paths and
> freeing allocated memories appropriately.
> 
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/trace/trace.c |   23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 473eb68..bbd86d2 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6277,7 +6277,7 @@ static int new_instance_create(const char *name)
>  		goto out_free_tr;
>  
>  	if (!alloc_cpumask_var(&tr->tracing_cpumask, GFP_KERNEL))
> -		goto out_free_tr;
> +		goto out_free_tr_name;
>  
>  	cpumask_copy(tr->tracing_cpumask, cpu_all_mask);
>  
> @@ -6291,16 +6291,16 @@ static int new_instance_create(const char *name)
>  	INIT_LIST_HEAD(&tr->events);
>  
>  	if (allocate_trace_buffers(tr, trace_buf_size) < 0)
> -		goto out_free_tr;
> +		goto out_free_cpumask_var;
>  
>  	tr->dir = debugfs_create_dir(name, trace_instance_dir);
>  	if (!tr->dir)
> -		goto out_free_tr;
> +		goto out_free_trace_buffers;
>  
>  	ret = event_trace_add_tracer(tr->dir, tr);
>  	if (ret) {
>  		debugfs_remove_recursive(tr->dir);
> -		goto out_free_tr;
> +		goto out_free_trace_buffers;
>  	}
>  
>  	init_tracer_debugfs(tr, tr->dir);
> @@ -6311,18 +6311,23 @@ static int new_instance_create(const char *name)
>  
>  	return 0;
>  
> - out_free_tr:
> -	if (tr->trace_buffer.buffer)
> -		ring_buffer_free(tr->trace_buffer.buffer);
> + out_free_trace_buffers:
> +	ring_buffer_free(tr->trace_buffer.buffer);
> +	free_percpu(tr->trace_buffer.data);
> +#ifdef CONFIG_TRACER_MAX_TRACE
> +	ring_buffer_free(tr->max_buffer.buffer);
> +	free_percpu(tr->max_buffer.data);
> +#endif

I think we need a free_trace_buffer() that complements
allocate_trace_buffer().

-- Steve

> + out_free_cpumask_var:
>  	free_cpumask_var(tr->tracing_cpumask);
> + out_free_tr_name:
>  	kfree(tr->name);
> + out_free_tr:
>  	kfree(tr);
> -
>   out_unlock:
>  	mutex_unlock(&trace_types_lock);
>  
>  	return ret;
> -
>  }
>  
>  static int instance_delete(const char *name)

Re: [PATCH ftrace/core 3/3] trace: Fix memory leak when new instance creation failed

[Steven Rostedt]

On Fri, 06 Jun 2014 07:35:22 +0900
Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com> wrote:

> Current new_instance_create() implements just two fail paths for four
> allocation operations. So, it can induce memory leak if new instance
> creation failed. This patch fixes it by defining all fail paths and
> freeing allocated memories appropriately.
> 

We don't need all the labels. The kfree() can handle NULL pointers.
Also, it's for a very unlikely case so we don't care about performance.

Here's the patch I'm adding:

-- Steve

>From 5ae90d9db393ac1b6189f8cb712ac5f526abd50e Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Date: Fri, 6 Jun 2014 00:01:46 -0400
Subject: [PATCH] tracing: Fix leak of ring buffer data when new instances
 creation fails

Yoshihiro Yunomae reported that the ring buffer data for a trace
instance does not get properly cleaned up when it fails. He proposed
a patch that manually cleaned the data up and addad a bunch of labels.
The labels are not needed because all trace array is allocated with
a kzalloc which initializes it to 0 and all kfree()s can take a NULL
pointer and will ignore it.

Adding a new helper function free_trace_buffers() that can also take
null buffers to free the buffers that were allocated by
allocate_trace_buffers().

Link: http://lkml.kernel.org/r/20140605223522.32311.31664.stgit@yunodevel

Reported-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e29edee..26cfff3 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6232,6 +6232,25 @@ static int allocate_trace_buffers(struct trace_array *tr, int size)
 	return 0;
 }
 
+static void free_trace_buffers(struct trace_array *tr)
+{
+	if (!tr)
+		return;
+
+	if (tr->trace_buffer.buffer) {
+		ring_buffer_free(tr->trace_buffer.buffer);
+		tr->trace_buffer.buffer = NULL;
+		free_percpu(tr->trace_buffer.data);
+	}
+
+#ifdef CONFIG_TRACER_MAX_TRACE
+	if (tr->max_buffer.buffer) {
+		ring_buffer_free(tr->max_buffer.buffer);
+		tr->max_buffer.buffer = NULL;
+	}
+#endif
+}
+
 static int new_instance_create(const char *name)
 {
 	struct trace_array *tr;
@@ -6290,8 +6309,7 @@ static int new_instance_create(const char *name)
 	return 0;
 
  out_free_tr:
-	if (tr->trace_buffer.buffer)
-		ring_buffer_free(tr->trace_buffer.buffer);
+	free_trace_buffers(tr);
 	free_cpumask_var(tr->tracing_cpumask);
 	kfree(tr->name);
 	kfree(tr);
-- 
1.8.1.4