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: [email protected]
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 <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: [email protected]
---
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");
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 <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: [email protected]
---
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: ");
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 <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: [email protected]
---
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)
(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 <[email protected]>
>
> Signed-off-by: Yoshihiro YUNOMAE <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: [email protected]
> ---
> 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: [email protected]
On Fri, 06 Jun 2014 07:35:17 +0900
Yoshihiro YUNOMAE <[email protected]> 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 <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: [email protected]
> ---
On Fri, 06 Jun 2014 07:35:22 +0900
Yoshihiro YUNOMAE <[email protected]> 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 <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: [email protected]
> ---
> 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)
On Fri, 06 Jun 2014 07:35:22 +0900
Yoshihiro YUNOMAE <[email protected]> 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)" <[email protected]>
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 <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
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
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 <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: [email protected]
---
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");
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 <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: [email protected]
> ---
> 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: [email protected]
On Fri, 06 Jun 2014 15:31:15 +0900
Yoshihiro YUNOMAE <[email protected]> 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