Skip to contentSkip to conversation

tracing: Some more last minute fixes

PATCH New Steven Rostedt 1 participant 9 messages Jul 3, 2013

Subsystems (2)
Stable branch Tracing
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover tracing: Some more last minute fixes

More fixes that need to go into linux-next before going mainline.

Still working on a fix for one more bug.

-- Steve

Steven Rostedt (Red Hat) (4):
tracing: Add trace_array_get/put() to event handling
tracing: Fix race between deleting buffer and setting events
tracing: Make tracer_tracing_{off,on,is_on}() static
tracing: Make tracing_open_generic_{tr,tc}() static

zhangwei(Jovi) (4):
uprobes: Fix return value in error handling path
tracing: Fix irqs-off tag display in syscall tracing
tracing: Remove TRACE_EVENT_TYPE enum definition
tracing: Remove ftrace() function

----

 kernel/trace/trace.c          |   19 +++-------
 kernel/trace/trace.h          |   14 ++------
 kernel/trace/trace_events.c   |   78 +++++++++++++++++++++++++++++++++++------
 kernel/trace/trace_syscalls.c |   21 ++++++++---
 kernel/trace/trace_uprobe.c   |    4 ++-
 5 files changed, 96 insertions(+), 40 deletions(-)
author maintainer
#1 lore raw

PATCH 1/8 tracing: Add trace_array_get/put() to event handling

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

Commit a695cb58162 "tracing: Prevent deleting instances when they are being read" tried to fix a race between deleting a trace instance and reading contents of a trace file. But it wasn't good enough. The following could crash the kernel:

 # cd /sys/kernel/debug/tracing/instances
 # ( while :; do mkdir foo; rmdir foo; done ) &
 # ( while :; do echo 1 > foo/events/sched/sched_switch 2> /dev/null; done ) &

Luckily this can only be done by root user, but it should be fixed regardless.

The problem is that a delete of the file can happen after the write to the event is opened, but before the enabling happens.

The solution is to make sure the trace_array is available before succeeding in opening for write, and incerment the ref counter while opened.

Now the instance can be deleted when the events are writing to the buffer, but the deletion of the instance will disable all events before the instance is actually deleted.

Cc [email protected] # 3.10
Reported-by Alexander Lam <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace.h        |    3 +++
 kernel/trace/trace_events.c |   55 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 54 insertions(+), 4 deletions(-)
kernel/trace/trace.h +3 -0
index 2c3cba59..c7fbf93 100644
@@ -226,6 +226,9 @@ extern struct list_head ftrace_trace_arrays;
 
 extern struct mutex trace_types_lock;
 
+extern int trace_array_get(struct trace_array *tr);
+extern void trace_array_put(struct trace_array *tr);
+
 /*
  * The global tracer (top) should be the first trace array added,
  * but we check the flag anyway.
kernel/trace/trace_events.c +51 -4

97 lines not on this page · raw message

Signature · 1 line
1.7.10.4
author maintainer
#2 lore raw

PATCH 2/8 tracing: Fix race between deleting buffer and setting events

In reply to Steven Rostedt

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

While analyzing the code, I discovered that there's a potential race between deleting a trace instance and setting events. There are a few races that can occur if events are being traced as the buffer is being deleted. Mostly the problem comes with freeing the descriptor used by the trace event callback. To prevent problems like this, the events are disabled before the buffer is deleted. The problem with the current solution is that the event_mutex is let go between disabling the events and freeing the files, which means that the events could be enabled again while the freeing takes place.

Cc [email protected] # 3.10
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_events.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)
kernel/trace/trace_events.c +17 -6
index 920e08f..7d85429 100644
@@ -441,14 +441,14 @@ static int tracing_release_generic_file(struct inode *inode, struct file *filp)
 /*
  * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
  */
-static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
-				  const char *sub, const char *event, int set)
+static int
+__ftrace_set_clr_event_nolock(struct trace_array *tr, const char *match,
+			      const char *sub, const char *event, int set)
 {
 	struct ftrace_event_file *file;
 	struct ftrace_event_call *call;
 	int ret = -EINVAL;
 
-	mutex_lock(&event_mutex);
 	list_for_each_entry(file, &tr->events, list) {
 
 		call = file->event_call;

@@ -474,6 +474,17 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
 
 		ret = 0;
 	}
+
+	return ret;
+}
+
+static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
+				  const char *sub, const char *event, int set)
+{
+	int ret;
+
+	mutex_lock(&event_mutex);
+	ret = __ftrace_set_clr_event_nolock(tr, match, sub, event, set);
 	mutex_unlock(&event_mutex);
 
 	return ret;

@@ -2408,11 +2419,11 @@ early_event_add_tracer(struct dentry *parent, struct trace_array *tr)
 
 int event_trace_del_tracer(struct trace_array *tr)
 {
-	/* Disable any running events */
-	__ftrace_set_clr_event(tr, NULL, NULL, NULL, 0);
-
 	mutex_lock(&event_mutex);
 
+	/* Disable any running events */
+	__ftrace_set_clr_event_nolock(tr, NULL, NULL, NULL, 0);
+
 	down_write(&trace_event_sem);
 	__trace_remove_event_dirs(tr);
 	debugfs_remove_recursive(tr->event_dir);
Signature · 1 line
1.7.10.4
author maintainer
#3 lore raw

PATCH 3/8 uprobes: Fix return value in error handling path

In reply to Steven Rostedt

From: "zhangwei(Jovi)" <[email protected]>

When wrong argument is passed into uprobe_events it does not return an error:

[root@jovi tracing]# echo 'p:myprobe /bin/bash' > uprobe_events [root@jovi tracing]#

The proper response is:

[root@jovi tracing]# echo 'p:myprobe /bin/bash' > uprobe_events -bash: echo: write error: Invalid argument

Link http://lkml.kernel.org/r/[email protected] open here
Cc Frederic Weisbecker <[email protected]>
Cc <[email protected]>
Cc [email protected] # 3.5+
Signed-off-by zhangwei(Jovi) <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_uprobe.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/trace/trace_uprobe.c +3 -1
index 32494fb0..d5d0cd3 100644
@@ -283,8 +283,10 @@ static int create_trace_uprobe(int argc, char **argv)
 		return -EINVAL;
 	}
 	arg = strchr(argv[1], ':');
-	if (!arg)
+	if (!arg) {
+		ret = -EINVAL;
 		goto fail_address_parse;
+	}
 
 	*arg++ = '\0';
 	filename = argv[1];
Signature · 1 line
1.7.10.4
author maintainer
#4 lore raw

PATCH 4/8 tracing: Fix irqs-off tag display in syscall tracing

In reply to Steven Rostedt

From: "zhangwei(Jovi)" <[email protected]>

All syscall tracing irqs-off tags are wrong, the syscall enter entry doesn't disable irqs.

[root@jovi tracing]#echo "syscalls:sys_enter_open" > set_event [root@jovi tracing]# cat trace
# tracer: nop
#
# entries-in-buffer/entries-written: 13/13 #P:2
#
# _-----=> irqs-off
# / _----=> need-resched

 #                            | / _---=> hardirq/softirq
 #                            || / _--=> preempt-depth
 #                            ||| /     delay
 #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
 #              | |       |   ||||       |         |
       irqbalance-513   [000] d... 56115.496766: sys_open(filename: 804e1a6, flags: 0, mode: 1b6)
       irqbalance-513   [000] d... 56115.497008: sys_open(filename: 804e1bb, flags: 0, mode: 1b6)
         sendmail-771   [000] d... 56115.827982: sys_open(filename: b770e6d1, flags: 0, mode: 1b6)

The reason is syscall tracing doesn't record irq_flags into buffer. The proper display is:

[root@jovi tracing]#echo "syscalls:sys_enter_open" > set_event [root@jovi tracing]# cat trace
# tracer: nop
#
# entries-in-buffer/entries-written: 14/14 #P:2
#
# _-----=> irqs-off
# / _----=> need-resched

 #                            | / _---=> hardirq/softirq
 #                            || / _--=> preempt-depth
 #                            ||| /     delay
 #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
 #              | |       |   ||||       |         |
       irqbalance-514   [001] ....    46.213921: sys_open(filename: 804e1a6, flags: 0, mode: 1b6)
       irqbalance-514   [001] ....    46.214160: sys_open(filename: 804e1bb, flags: 0, mode: 1b6)
            <...>-920   [001] ....    47.307260: sys_open(filename: 4e82a0c5, flags: 80000, mode: 0)
Link http://lkml.kernel.org/r/[email protected] open here
Cc [email protected] # 2.6.35
Signed-off-by zhangwei(Jovi) <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_syscalls.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
kernel/trace/trace_syscalls.c +17 -4
index 8f2ac73..322e164 100644
@@ -306,6 +306,8 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id)
 	struct syscall_metadata *sys_data;
 	struct ring_buffer_event *event;
 	struct ring_buffer *buffer;
+	unsigned long irq_flags;
+	int pc;
 	int syscall_nr;
 	int size;
 

@@ -321,9 +323,12 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id)
 
 	size = sizeof(*entry) + sizeof(unsigned long) * sys_data->nb_args;
 
+	local_save_flags(irq_flags);
+	pc = preempt_count();
+
 	buffer = tr->trace_buffer.buffer;
 	event = trace_buffer_lock_reserve(buffer,
-			sys_data->enter_event->event.type, size, 0, 0);
+			sys_data->enter_event->event.type, size, irq_flags, pc);
 	if (!event)
 		return;
 

@@ -333,7 +338,8 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id)
 
 	if (!filter_current_check_discard(buffer, sys_data->enter_event,
 					  entry, event))
-		trace_current_buffer_unlock_commit(buffer, event, 0, 0);
+		trace_current_buffer_unlock_commit(buffer, event,
+						   irq_flags, pc);
 }
 
 static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)

@@ -343,6 +349,8 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)
 	struct syscall_metadata *sys_data;
 	struct ring_buffer_event *event;
 	struct ring_buffer *buffer;
+	unsigned long irq_flags;
+	int pc;
 	int syscall_nr;
 
 	syscall_nr = trace_get_syscall_nr(current, regs);

@@ -355,9 +363,13 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)
 	if (!sys_data)
 		return;
 
+	local_save_flags(irq_flags);
+	pc = preempt_count();
+
 	buffer = tr->trace_buffer.buffer;
 	event = trace_buffer_lock_reserve(buffer,
-			sys_data->exit_event->event.type, sizeof(*entry), 0, 0);
+			sys_data->exit_event->event.type, sizeof(*entry),
+			irq_flags, pc);
 	if (!event)
 		return;
 

@@ -367,7 +379,8 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)
 
 	if (!filter_current_check_discard(buffer, sys_data->exit_event,
 					  entry, event))
-		trace_current_buffer_unlock_commit(buffer, event, 0, 0);
+		trace_current_buffer_unlock_commit(buffer, event,
+						   irq_flags, pc);
 }
 
 static int reg_event_syscall_enter(struct ftrace_event_file *file,
Signature · 1 line
1.7.10.4
author maintainer
#5 lore raw

PATCH 5/8 tracing: Make tracer_tracing_{off,on,is_on}() static

In reply to Steven Rostedt

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

I have patches that will use tracer_tracing_on/off/is_on() in other files, but as they are not ready to be merged yet, and Fengguang Wu's sparse scripts pointed out that these functions were not declared anywhere, I'll make them static for now.

When these functions are required to be used elsewhere, I'll remove the static then.

Reported-by kbuild test robot <[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 6d9bd9b..48aceb8 100644
@@ -387,7 +387,7 @@ unsigned long trace_flags = TRACE_ITER_PRINT_PARENT | TRACE_ITER_PRINTK |
 	TRACE_ITER_GRAPH_TIME | TRACE_ITER_RECORD_CMD | TRACE_ITER_OVERWRITE |
 	TRACE_ITER_IRQ_INFO | TRACE_ITER_MARKERS | TRACE_ITER_FUNCTION;
 
-void tracer_tracing_on(struct trace_array *tr)
+static void tracer_tracing_on(struct trace_array *tr)
 {
 	if (tr->trace_buffer.buffer)
 		ring_buffer_record_on(tr->trace_buffer.buffer);

@@ -606,7 +606,7 @@ void tracing_snapshot_alloc(void)
 EXPORT_SYMBOL_GPL(tracing_snapshot_alloc);
 #endif /* CONFIG_TRACER_SNAPSHOT */
 
-void tracer_tracing_off(struct trace_array *tr)
+static void tracer_tracing_off(struct trace_array *tr)
 {
 	if (tr->trace_buffer.buffer)
 		ring_buffer_record_off(tr->trace_buffer.buffer);

@@ -649,7 +649,7 @@ void disable_trace_on_warning(void)
  *
  * Shows real state of the ring buffer if it is enabled or not.
  */
-int tracer_tracing_is_on(struct trace_array *tr)
+static int tracer_tracing_is_on(struct trace_array *tr)
 {
 	if (tr->trace_buffer.buffer)
 		return ring_buffer_record_is_on(tr->trace_buffer.buffer);
Signature · 1 line
1.7.10.4
author maintainer
#6 lore raw

PATCH 6/8 tracing: Remove TRACE_EVENT_TYPE enum definition

In reply to Steven Rostedt

From: "zhangwei(Jovi)" <[email protected]>

TRACE_EVENT_TYPE enum is not used at present, remove it.

Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by zhangwei(Jovi) <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace.h |    6 ------
 1 file changed, 6 deletions(-)
kernel/trace/trace.h +0 -6
index c7fbf93..1cbba04 100644
@@ -907,12 +907,6 @@ static inline void trace_branch_disable(void)
 /* set ring buffers to default size if not already done so */
 int tracing_update_buffers(void);
 
-/* trace event type bit fields, not numeric */
-enum {
-	TRACE_EVENT_TYPE_PRINTF		= 1,
-	TRACE_EVENT_TYPE_RAW		= 2,
-};
-
 struct ftrace_event_field {
 	struct list_head	link;
 	const char		*name;
Signature · 1 line
1.7.10.4
author maintainer
#7 lore raw

PATCH 7/8 tracing: Remove ftrace() function

In reply to Steven Rostedt

From: "zhangwei(Jovi)" <[email protected]>

The only caller of function ftrace(...) was removed a long time ago, so remove the function body as well.

Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by zhangwei(Jovi) <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace.c |    9 ---------
 kernel/trace/trace.h |    5 -----
 2 files changed, 14 deletions(-)
kernel/trace/trace.c +0 -9
index 48aceb8..f6fed9e 100644
@@ -1637,15 +1637,6 @@ trace_function(struct trace_array *tr,
 		__buffer_unlock_commit(buffer, event);
 }
 
-void
-ftrace(struct trace_array *tr, struct trace_array_cpu *data,
-       unsigned long ip, unsigned long parent_ip, unsigned long flags,
-       int pc)
-{
-	if (likely(!atomic_read(&data->disabled)))
-		trace_function(tr, ip, parent_ip, flags, pc);
-}
-
 #ifdef CONFIG_STACKTRACE
 
 #define FTRACE_STACK_MAX_ENTRIES (PAGE_SIZE / sizeof(unsigned long))
kernel/trace/trace.h +0 -5
index 1cbba04..a4ed382 100644
@@ -559,11 +559,6 @@ void tracing_iter_reset(struct trace_iterator *iter, int cpu);
 
 void poll_wait_pipe(struct trace_iterator *iter);
 
-void ftrace(struct trace_array *tr,
-			    struct trace_array_cpu *data,
-			    unsigned long ip,
-			    unsigned long parent_ip,
-			    unsigned long flags, int pc);
 void tracing_sched_switch_trace(struct trace_array *tr,
 				struct task_struct *prev,
 				struct task_struct *next,
Signature · 1 line
1.7.10.4
author maintainer
#8 lore raw

PATCH 8/8 tracing: Make tracing_open_generic_{tr,tc}() static

In reply to Steven Rostedt

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

I have patches that will use tracing_open_generic_tr/tc() in other files, but as they are not ready to be merged yet, and Fengguang Wu's sparse scripts pointed out that these functions were not declared anywhere, I'll make them static for now.

When these functions are required to be used elsewhere, I'll remove the static then.

Reported-by kbuild test robot <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/trace/trace.c +2 -2
index f6fed9e..dc473b5 100644
@@ -2960,7 +2960,7 @@ int tracing_open_generic(struct inode *inode, struct file *filp)
  * Open and update trace_array ref count.
  * Must have the current trace_array passed to it.
  */
-int tracing_open_generic_tr(struct inode *inode, struct file *filp)
+static int tracing_open_generic_tr(struct inode *inode, struct file *filp)
 {
 	struct trace_array *tr = inode->i_private;
 

@@ -2976,7 +2976,7 @@ int tracing_open_generic_tr(struct inode *inode, struct file *filp)
 	
 }
 
-int tracing_open_generic_tc(struct inode *inode, struct file *filp)
+static int tracing_open_generic_tc(struct inode *inode, struct file *filp)
 {
 	struct trace_cpu *tc = inode->i_private;
 	struct trace_array *tr = tc->tr;
Signature · 1 line
1.7.10.4