Skip to contentSkip to conversation

ftrace: Fix three small bugs

PATCH In review Yoshihiro YUNOMAE 3 participants 11 messages Jun 5, 2014 – Jun 6, 2014 Tracing

lore
Share & tools
lore
#0 lore raw

cover ftrace: Fix three small bugs

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(-)
Signature · 4 lines
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
#1 lore raw

PATCH 1/3 trace/event: Return error if ftrace_trace_arrays is empty list

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.

---
 kernel/trace/trace.h        |    3 +++
 kernel/trace/trace_events.c |   13 +++++++++++++
 2 files changed, 16 insertions(+)
kernel/trace/trace.h +3 -0
index 217207a..9e82551 100644
@@ -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));
kernel/trace/trace_events.c +13 -0
index 3ddfd8f..1349870 100644
@@ -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");
 
maintainer
#2 lore raw

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

Quoted 10 lines · On Fri, 06 Jun 2014 07:35:17 +0900 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.

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

Quoted 6 lines

---

#3 lore raw

PATCH 1/1 tracing: Remove return value in event_trace_self_tests() when top_trace_array() returns NULL

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

---
 kernel/trace/trace_events.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace_events.c +1 -1
index 1349870..f99e0b3 100644
@@ -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");
 
#4 lore raw

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

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

Quoted 26 lines · Yoshihiro YUNOMAE

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

---
  kernel/trace/trace_events.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace_events.c +1 -1
index 1349870..f99e0b3 100644
@@ -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");
 
Signature · 4 lines
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
maintainer
#5 lore raw

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

Quoted 6 lines · On Fri, 06 Jun 2014 15:31:15 +0900 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?

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

#6 lore raw

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

In reply to 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.

Cc Steven Rostedt <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Masami Hiramatsu <[email protected]>
---
 kernel/trace/trace_kprobe.c |    3 +++
 1 file changed, 3 insertions(+)
kernel/trace/trace_kprobe.c +3 -0
index 903ae28..ef2fba1 100644
@@ -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: ");
Acked-by on v1 2/3
#7 lore raw

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

Quoted 3 lines · 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.

Looks good to me, thanks! :)

Quoted 26 lines
Cc Steven Rostedt <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Masami Hiramatsu <[email protected]>
---
 kernel/trace/trace_kprobe.c |    3 +++
 1 file changed, 3 insertions(+)
kernel/trace/trace_kprobe.c +3 -0
index 903ae28..ef2fba1 100644
@@ -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: ");
Signature · 4 lines
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
#8 lore raw

PATCH 3/3 trace: Fix memory leak when new instance creation failed

In reply to 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.

---
 kernel/trace/trace.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
kernel/trace/trace.c +14 -9
index 473eb68..bbd86d2 100644
@@ -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)
maintainer
#9 lore raw

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

Quoted 50 lines · On Fri, 06 Jun 2014 07:35:22 +0900 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.

---
 kernel/trace/trace.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
kernel/trace/trace.c +4 -4
index 473eb68..bbd86d2 100644
@@ -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;
 

kernel/trace/trace.c static int new_instance_create(const char *name) · line 6320

Quoted 10 lines
kernel/trace/trace.c +7 -3
- 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

Quoted 15 lines
kernel/trace/trace.c +3 -2
+ 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)
maintainer
#10 lore raw

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

In reply to Yoshihiro YUNOMAE
Quoted 5 lines · On Fri, 06 Jun 2014 07:35:22 +0900 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.

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

Quoted 1 line

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

---
 kernel/trace/trace.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)
kernel/trace/trace.c +20 -2
index e29edee..26cfff3 100644
@@ -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);
Signature · 1 line
1.8.1.4