tracing: Cleanups and fixes for 3.16

PATCH New Steven Rostedt 1 participant 5 messages Jun 11, 2014

Subsystems (2)

Tracing Tracing ring buffer

lore

Share & tools

lore

Steven Rostedt

author maintainer

2014-06-11 02:01 UTC #0 lore raw

cover tracing: Cleanups and fixes for 3.16

Thanks to Namhyung Kim who pointed out some slight things with the code that went to Linus already, I have some early cleanups.

There's also a bug that needs to be fixed in the ring buffer waiter logic.

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
for-next

Head SHA1: da9c3413a27be5ba6f996e90495c836dd30b8841

Namhyung Kim (1)
tracing: Cleanup saved_cmdlines_size changes

Steven Rostedt (Red Hat) (3)
ring-buffer: Check if buffer exists before polling
tracing: Fix leak of per cpu max data in instances
tracing: Fix check of ftrace_trace_arrays list_empty() check

----

 include/linux/ring_buffer.h |    2 -
 kernel/trace/ring_buffer.c  |    5 +++-
 kernel/trace/trace.c        |   49 +++++++++++++++++++++++++++-----------------
 kernel/trace/trace.h        |    2 -
 4 files changed, 37 insertions(+), 21 deletions(-)

Steven Rostedt

author maintainer

2014-06-11 02:02 UTC #1 lore raw

PATCH 1/4 ring-buffer: Check if buffer exists before polling

From: "Steven Rostedt (Red Hat)" <[email protected]>

The per_cpu buffers are created one per possible CPU. But these do not mean that those CPUs are online, nor do they even exist.

With the addition of the ring buffer polling, it assumes that the caller polls on an existing buffer. But this is not the case if the user reads trace_pipe from a CPU that does not exist, and this causes the kernel to crash.

Simple fix is to check the cpu against buffer bitmask against to see if the buffer was allocated or not and return -ENODEV if it is not.

More updates were done to pass the -ENODEV back up to userspace.

Link http://lkml.kernel.org/r/[email protected] open here

Reported-by Sasha Levin <[email protected]>

Cc [email protected] # 3.10+

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/ring_buffer.h |  2 +-
 kernel/trace/ring_buffer.c  |  5 ++++-
 kernel/trace/trace.c        | 22 ++++++++++++++++------
 3 files changed, 21 insertions(+), 8 deletions(-)

include/linux/ring_buffer.h +1 -1

index d69cf637a15a..49a4d6f59108 100644
@@ -97,7 +97,7 @@ __ring_buffer_alloc(unsigned long size, unsigned flags, struct lock_class_key *k
 	__ring_buffer_alloc((size), (flags), &__key);	\
 })
 
-void ring_buffer_wait(struct ring_buffer *buffer, int cpu);
+int ring_buffer_wait(struct ring_buffer *buffer, int cpu);
 int ring_buffer_poll_wait(struct ring_buffer *buffer, int cpu,
 			  struct file *filp, poll_table *poll_table);

kernel/trace/ring_buffer.c +4 -1

index c634868c2921..7c56c3d06943 100644
@@ -543,7 +543,7 @@ static void rb_wake_up_waiters(struct irq_work *work)
  * as data is added to any of the @buffer's cpu buffers. Otherwise
  * it will wait for data to be added to a specific cpu buffer.
  */
-void ring_buffer_wait(struct ring_buffer *buffer, int cpu)
+int ring_buffer_wait(struct ring_buffer *buffer, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
 	DEFINE_WAIT(wait);

@@ -557,6 +557,8 @@ void ring_buffer_wait(struct ring_buffer *buffer, int cpu)
 	if (cpu == RING_BUFFER_ALL_CPUS)
 		work = &buffer->irq_work;
 	else {
+		if (!cpumask_test_cpu(cpu, buffer->cpumask))
+			return -ENODEV;
 		cpu_buffer = buffer->buffers[cpu];
 		work = &cpu_buffer->irq_work;
 	}

@@ -591,6 +593,7 @@ void ring_buffer_wait(struct ring_buffer *buffer, int cpu)
 		schedule();
 
 	finish_wait(&work->waiters, &wait);
+	return 0;
 }
 
 /**

kernel/trace/trace.c +16 -6

index 16f7038d1f4d..56422f1decba 100644
@@ -1085,13 +1085,13 @@ update_max_tr_single(struct trace_array *tr, struct task_struct *tsk, int cpu)
 }
 #endif /* CONFIG_TRACER_MAX_TRACE */
 
-static void wait_on_pipe(struct trace_iterator *iter)
+static int wait_on_pipe(struct trace_iterator *iter)
 {
 	/* Iterators are static, they should be filled or empty */
 	if (trace_buffer_iter(iter, iter->cpu_file))
-		return;
+		return 0;
 
-	ring_buffer_wait(iter->trace_buffer->buffer, iter->cpu_file);
+	return ring_buffer_wait(iter->trace_buffer->buffer, iter->cpu_file);
 }
 
 #ifdef CONFIG_FTRACE_STARTUP_TEST

@@ -4378,6 +4378,7 @@ tracing_poll_pipe(struct file *filp, poll_table *poll_table)
 static int tracing_wait_pipe(struct file *filp)
 {
 	struct trace_iterator *iter = filp->private_data;
+	int ret;
 
 	while (trace_empty(iter)) {

@@ -4399,10 +4400,13 @@ static int tracing_wait_pipe(struct file *filp)
 
 		mutex_unlock(&iter->mutex);
 
-		wait_on_pipe(iter);
+		ret = wait_on_pipe(iter);
 
 		mutex_lock(&iter->mutex);
 
+		if (ret)
+			return ret;
+
 		if (signal_pending(current))
 			return -EINTR;
 	}

@@ -5327,8 +5331,12 @@ tracing_buffers_read(struct file *filp, char __user *ubuf,
 				goto out_unlock;
 			}
 			mutex_unlock(&trace_types_lock);
-			wait_on_pipe(iter);
+			ret = wait_on_pipe(iter);
 			mutex_lock(&trace_types_lock);
+			if (ret) {
+				size = ret;
+				goto out_unlock;
+			}
 			if (signal_pending(current)) {
 				size = -EINTR;
 				goto out_unlock;

@@ -5538,8 +5546,10 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 			goto out;
 		}
 		mutex_unlock(&trace_types_lock);
-		wait_on_pipe(iter);
+		ret = wait_on_pipe(iter);
 		mutex_lock(&trace_types_lock);
+		if (ret)
+			goto out;
 		if (signal_pending(current)) {
 			ret = -EINTR;
 			goto out;

Signature · 1 line

2.0.0.rc2

Steven Rostedt

author maintainer

2014-06-11 02:01 UTC #2 lore raw

PATCH 2/4 tracing: Cleanup saved_cmdlines_size changes

In reply to Steven Rostedt

From: Namhyung Kim <[email protected]>

The recent addition of saved_cmdlines_size file had some remaining (minor - mostly coding style) issues. Fix them by passing pointer name to sizeof() and using scnprintf().

Link http://lkml.kernel.org/p/[email protected] open here

Cc Namhyung Kim <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Yoshihiro YUNOMAE <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/trace/trace.c +3 -3

index 56422f1decba..2b458c60e0da 100644
@@ -1338,7 +1338,7 @@ static int trace_create_savedcmd(void)
 {
 	int ret;
 
-	savedcmd = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
+	savedcmd = kmalloc(sizeof(*savedcmd), GFP_KERNEL);
 	if (!savedcmd)
 		return -ENOMEM;

@@ -3840,7 +3840,7 @@ tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
 	int r;
 
 	arch_spin_lock(&trace_cmdline_lock);
-	r = sprintf(buf, "%u\n", savedcmd->cmdline_num);
+	r = scnprintf(buf, sizeof(buf), "%u\n", savedcmd->cmdline_num);
 	arch_spin_unlock(&trace_cmdline_lock);
 
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);

@@ -3857,7 +3857,7 @@ static int tracing_resize_saved_cmdlines(unsigned int val)
 {
 	struct saved_cmdlines_buffer *s, *savedcmd_temp;
 
-	s = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
+	s = kmalloc(sizeof(*s), GFP_KERNEL);
 	if (!s)
 		return -ENOMEM;

Signature · 1 line

2.0.0.rc2

Steven Rostedt

author maintainer

2014-06-11 02:01 UTC #3 lore raw

PATCH 3/4 tracing: Fix leak of per cpu max data in instances

In reply to Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

The freeing of an instance, if max data is configured, there will be per cpu data structures created. But these are not freed when the instance is deleted, which causes a memory leak.

A new helper function is added that frees the individual buffers within a trace array, instead of duplicating the code. This way changes made for one are applied to the other (normal buffer vs max buffer).

Link http://lkml.kernel.org/r/[email protected] open here

Reported-by Namhyung Kim <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

kernel/trace/trace.c +12 -9

index 2b458c60e0da..384ede311717 100644
@@ -6242,22 +6242,25 @@ static int allocate_trace_buffers(struct trace_array *tr, int size)
 	return 0;
 }
 
+static void free_trace_buffer(struct trace_buffer *buf)
+{
+	if (buf->buffer) {
+		ring_buffer_free(buf->buffer);
+		buf->buffer = NULL;
+		free_percpu(buf->data);
+		buf->data = NULL;
+	}
+}
+
 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);
-	}
+	free_trace_buffer(&tr->trace_buffer);
 
 #ifdef CONFIG_TRACER_MAX_TRACE
-	if (tr->max_buffer.buffer) {
-		ring_buffer_free(tr->max_buffer.buffer);
-		tr->max_buffer.buffer = NULL;
-	}
+	free_trace_buffer(&tr->max_buffer);
 #endif
 }

Signature · 1 line

2.0.0.rc2

Steven Rostedt

author maintainer

2014-06-11 02:01 UTC #4 lore raw

PATCH 4/4 tracing: Fix check of ftrace_trace_arrays list_empty() check

In reply to Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

The check that tests if ftrace_trace_arrays is empty in top_trace_array(), uses the .prev pointer:

if (list_empty(ftrace_trace_arrays.prev))

instead of testing the variable itself:

if (list_empty(&ftrace_trace_arrays))

Although it is technically correct, it is awkward and confusing. Use the proper method.

Link http://lkml.kernel.org/r/[email protected] open here

Reported-by Namhyung Kim <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/trace/trace.h +1 -1

index 9e82551dd566..9258f5a815db 100644
@@ -252,7 +252,7 @@ static inline struct trace_array *top_trace_array(void)
 {
 	struct trace_array *tr;
 
-	if (list_empty(ftrace_trace_arrays.prev))
+	if (list_empty(&ftrace_trace_arrays))
 		return NULL;
 
 	tr = list_entry(ftrace_trace_arrays.prev,

Signature · 1 line

2.0.0.rc2