perf_event: fix race in perf_swevent_get_recursion_context()

Those function's return value is meaningless

Signed-off-by Xiao Guangrong <[email protected]>

---
 kernel/trace/trace_kprobe.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

kernel/trace/trace_kprobe.c +14 -14

index 6d478c1..5869819 100644
@@ -942,7 +942,7 @@ static const struct file_operations kprobe_profile_ops = {
 };
 
 /* Kprobe handler */
-static __kprobes int kprobe_trace_func(struct kprobe *kp, struct pt_regs *regs)
+static __kprobes void kprobe_trace_func(struct kprobe *kp, struct pt_regs *regs)
 {
 	struct trace_probe *tp = container_of(kp, struct trace_probe, rp.kp);
 	struct kprobe_trace_entry *entry;

@@ -962,7 +962,7 @@ static __kprobes int kprobe_trace_func(struct kprobe *kp, struct pt_regs *regs)
 	event = trace_current_buffer_lock_reserve(&buffer, call->id, size,
 						  irq_flags, pc);
 	if (!event)
-		return 0;
+		return ;
 
 	entry = ring_buffer_event_data(event);
 	entry->nargs = tp->nr_args;

@@ -972,11 +972,11 @@ static __kprobes int kprobe_trace_func(struct kprobe *kp, struct pt_regs *regs)
 
 	if (!filter_current_check_discard(buffer, call, entry, event))
 		trace_nowake_buffer_unlock_commit(buffer, event, irq_flags, pc);
-	return 0;
+	return ;
 }
 
 /* Kretprobe handler */
-static __kprobes int kretprobe_trace_func(struct kretprobe_instance *ri,
+static __kprobes void kretprobe_trace_func(struct kretprobe_instance *ri,
 					  struct pt_regs *regs)
 {
 	struct trace_probe *tp = container_of(ri->rp, struct trace_probe, rp);

@@ -995,7 +995,7 @@ static __kprobes int kretprobe_trace_func(struct kretprobe_instance *ri,
 	event = trace_current_buffer_lock_reserve(&buffer, call->id, size,
 						  irq_flags, pc);
 	if (!event)
-		return 0;
+		return ;
 
 	entry = ring_buffer_event_data(event);
 	entry->nargs = tp->nr_args;

@@ -1007,7 +1007,7 @@ static __kprobes int kretprobe_trace_func(struct kretprobe_instance *ri,
 	if (!filter_current_check_discard(buffer, call, entry, event))
 		trace_nowake_buffer_unlock_commit(buffer, event, irq_flags, pc);
 
-	return 0;
+	return ;
 }
 
 /* Event entry printers */

@@ -1217,7 +1217,7 @@ static int set_print_fmt(struct trace_probe *tp)
 #ifdef CONFIG_PERF_EVENTS
 
 /* Kprobe profile handler */
-static __kprobes int kprobe_profile_func(struct kprobe *kp,
+static __kprobes void kprobe_profile_func(struct kprobe *kp,
 					 struct pt_regs *regs)
 {
 	struct trace_probe *tp = container_of(kp, struct trace_probe, rp.kp);

@@ -1232,11 +1232,11 @@ static __kprobes int kprobe_profile_func(struct kprobe *kp,
 	size -= sizeof(u32);
 	if (WARN_ONCE(size > FTRACE_MAX_PROFILE_SIZE,
 		     "profile buffer not large enough"))
-		return 0;
+		return ;
 
 	entry = ftrace_profile_buf_begin(size, call->id, &rctx, &irq_flags);
 	if (!entry)
-		return 0;
+		return ;
 
 	entry->nargs = tp->nr_args;
 	entry->ip = (unsigned long)kp->addr;

@@ -1245,11 +1245,11 @@ static __kprobes int kprobe_profile_func(struct kprobe *kp,
 
 	ftrace_profile_buf_end(entry, size, rctx, entry->ip, 1, irq_flags);
 
-	return 0;
+	return ;
 }
 
 /* Kretprobe profile handler */
-static __kprobes int kretprobe_profile_func(struct kretprobe_instance *ri,
+static __kprobes void kretprobe_profile_func(struct kretprobe_instance *ri,
 					    struct pt_regs *regs)
 {
 	struct trace_probe *tp = container_of(ri->rp, struct trace_probe, rp);

@@ -1264,11 +1264,11 @@ static __kprobes int kretprobe_profile_func(struct kretprobe_instance *ri,
 	size -= sizeof(u32);
 	if (WARN_ONCE(size > FTRACE_MAX_PROFILE_SIZE,
 		     "profile buffer not large enough"))
-		return 0;
+		return ;
 
 	entry = ftrace_profile_buf_begin(size, call->id, &rctx, &irq_flags);
 	if (!entry)
-		return 0;
+		return ;
 
 	entry->nargs = tp->nr_args;
 	entry->func = (unsigned long)tp->rp.kp.addr;

@@ -1278,7 +1278,7 @@ static __kprobes int kretprobe_profile_func(struct kretprobe_instance *ri,
 
 	ftrace_profile_buf_end(entry, size, rctx, entry->ret_ip, 1, irq_flags);
 
-	return 0;
+	return ;
 }
 
 static int probe_profile_enable(struct ftrace_event_call *call)

It only disable preemption in perf_swevent_get_recursion_context(), it can't avoid race of hard-irq and NMI since they are nested that will re-enter this path and make the recursion value chaotic

In this patch, we use atomic operation to avoid it and reduce cpu_ctx->recursion size, it also make this patch no need disable preemption

Quoted 7 lines · Xiao Guangrong

It only disable preemption in perf_swevent_get_recursion_context(), it can't avoid race of hard-irq and NMI since they are nested that will re-enter this path and make the recursion value chaotic

In this patch, we use atomic operation to avoid it and reduce cpu_ctx->recursion size, it also make this patch no need disable preemption

NAK

Introduce ftrace_profile_buf_begin() and ftrace_profile_buf_end() to operate event profile buffer, clean up redundant code

Signed-off-by Xiao Guangrong <[email protected]>

kernel/trace/trace_event_profile.c +45 -5

index 9e25573..f0fa16b 100644
@@ -9,11 +9,8 @@
 #include "trace.h"
 
 
-char *perf_trace_buf;
-EXPORT_SYMBOL_GPL(perf_trace_buf);
-
-char *perf_trace_buf_nmi;
-EXPORT_SYMBOL_GPL(perf_trace_buf_nmi);
+static char *perf_trace_buf;
+static char *perf_trace_buf_nmi;
 
 typedef typeof(char [FTRACE_MAX_PROFILE_SIZE]) perf_trace_t ;
 

@@ -120,3 +117,56 @@ void ftrace_profile_disable(int event_id)
 	}
 	mutex_unlock(&event_mutex);
 }
+
+void *ftrace_profile_buf_begin(int size, unsigned short type, int *rctxp,
+			       unsigned long *irq_flags)
+{
+	struct trace_entry *entry;
+	char *trace_buf, *raw_data;
+	int pc, cpu;
+
+	pc = preempt_count();
+
+	/* Protect the per cpu buffer, begin the rcu read side */
+	local_irq_save(*irq_flags);
+
+	*rctxp = perf_swevent_get_recursion_context();
+	if (*rctxp < 0)
+		goto err_recursion;
+
+	cpu = smp_processor_id();
+
+	if (in_nmi())
+		trace_buf = rcu_dereference(perf_trace_buf_nmi);
+	else
+		trace_buf = rcu_dereference(perf_trace_buf);
+
+	if (!trace_buf)
+		goto err;
+
+	raw_data = per_cpu_ptr(trace_buf, cpu);
+
+	/* zero the dead bytes from align to not leak stack to user */
+	*(u64 *)(&raw_data[size - sizeof(u64)]) = 0ULL;
+
+	entry = (struct trace_entry *)raw_data;
+	tracing_generic_entry_update(entry, *irq_flags, pc);
+	entry->type = type;
+
+	return raw_data;
+err:
+	perf_swevent_put_recursion_context(*rctxp);
+err_recursion:
+	local_irq_restore(*irq_flags);
+	return NULL;
+}

Quoted 4 lines · Xiao Guangrong

Introduce ftrace_profile_buf_begin() and ftrace_profile_buf_end() to operate event profile buffer, clean up redundant code

Signed-off-by Xiao Guangrong <[email protected]>

[...]

Quoted 65 lines

kernel/trace/trace_event_profile.c +45 -5

index 9e25573..f0fa16b 100644
@@ -9,11 +9,8 @@
 #include "trace.h"
 
 
-char *perf_trace_buf;
-EXPORT_SYMBOL_GPL(perf_trace_buf);
-
-char *perf_trace_buf_nmi;
-EXPORT_SYMBOL_GPL(perf_trace_buf_nmi);
+static char *perf_trace_buf;
+static char *perf_trace_buf_nmi;
 
 typedef typeof(char [FTRACE_MAX_PROFILE_SIZE]) perf_trace_t ;
 

@@ -120,3 +117,56 @@ void ftrace_profile_disable(int event_id)
 	}
 	mutex_unlock(&event_mutex);
 }
+
+void *ftrace_profile_buf_begin(int size, unsigned short type, int *rctxp,
+			       unsigned long *irq_flags)
+{
+	struct trace_entry *entry;
+	char *trace_buf, *raw_data;
+	int pc, cpu;
+
+	pc = preempt_count();
+
+	/* Protect the per cpu buffer, begin the rcu read side */
+	local_irq_save(*irq_flags);
+
+	*rctxp = perf_swevent_get_recursion_context();
+	if (*rctxp < 0)
+		goto err_recursion;
+
+	cpu = smp_processor_id();
+
+	if (in_nmi())
+		trace_buf = rcu_dereference(perf_trace_buf_nmi);
+	else
+		trace_buf = rcu_dereference(perf_trace_buf);
+
+	if (!trace_buf)
+		goto err;
+
+	raw_data = per_cpu_ptr(trace_buf, cpu);
+
+	/* zero the dead bytes from align to not leak stack to user */
+	*(u64 *)(&raw_data[size - sizeof(u64)]) = 0ULL;
+
+	entry = (struct trace_entry *)raw_data;
+	tracing_generic_entry_update(entry, *irq_flags, pc);
+	entry->type = type;
+
+	return raw_data;
+err:
+	perf_swevent_put_recursion_context(*rctxp);
+err_recursion:
+	local_irq_restore(*irq_flags);
+	return NULL;
+}

kernel/trace/trace_event_profile.c void ftrace_profile_disable(int event_id) · line 172

Quoted 10 lines

kernel/trace/trace_event_profile.c +10 -0

+
+void ftrace_profile_buf_end(void *raw_data, int size, int rctx, u64 addr,
+			    u64 count, unsigned long irq_flags)
+{
+	struct trace_entry *entry = raw_data;
+
+	perf_tp_event(entry->type, addr, count, raw_data, size);
+	perf_swevent_put_recursion_context(rctx);
+	local_irq_restore(irq_flags);
+}

Hmm, could you make it inline-functions or add __kprobes? Because it is called from kprobes, we don't want to probe the function which will be called from kprobes handlers itself.

(IMHO, from the viewpoint of performance, inline-function
could be better.)

Thank you,

Quoted 7 lines · Masami Hiramatsu

Hmm, could you make it inline-functions or add __kprobes? Because it is called from kprobes, we don't want to probe the function which will be called from kprobes handlers itself.

(IMHO, from the viewpoint of performance, inline-function
could be better.)

Quoted 1 line

Thank you,

Yeah, may be inline ftrace_profile_buf_end, would be better. But we shouldn't inline ftrace_profile_buf_begin() I guess, considering its size.

While at it, may be let's choose more verbose names like

ftrace_profile_buf_fill() and ftrace_profile_buf_submit().

Also, profile is a bit of a misnomer. Not a problem since ftrace_profile_templ_##call() is already a misnomer, but we should start a bit of a rename. Sometimes, perf only profiles trace events as counters and sometimes it records the raw samples too.

So, as more generic names, I would suggest:

ftrace_perf_buf_fill() and ftrace_perf_buf_submit().

Quoted 3 lines

So, as more generic names, I would suggest:

ftrace_perf_buf_fill() and ftrace_perf_buf_submit().

Actual filling buffer is done in the profile handlers, so I think ftrace_perf_buf_prepare() may be better :-) ftrace_perf_buf_submit is good to me:-)

Quoted 8 lines · Frederic Weisbecker

Quoted 7 lines · Masami Hiramatsu

Hmm, could you make it inline-functions or add __kprobes? Because it is called from kprobes, we don't want to probe the function which will be called from kprobes handlers itself.

(IMHO, from the viewpoint of performance, inline-function
could be better.)

Quoted 6 lines

Quoted 1 line

Thank you,

Yeah, may be inline ftrace_profile_buf_end, would be better. But we shouldn't inline ftrace_profile_buf_begin() I guess, considering its size.

Indeed, especially for events...

Quoted 11 lines

While at it, may be let's choose more verbose names like

ftrace_profile_buf_fill() and ftrace_profile_buf_submit().

Also, profile is a bit of a misnomer. Not a problem since ftrace_profile_templ_##call() is already a misnomer, but we should start a bit of a rename. Sometimes, perf only profiles trace events as counters and sometimes it records the raw samples too.

Quoted 3 lines

So, as more generic names, I would suggest:

ftrace_perf_buf_fill() and ftrace_perf_buf_submit().

Actual filling buffer is done in the profile handlers, so I think ftrace_perf_buf_prepare() may be better :-) ftrace_perf_buf_submit is good to me:-)

Quoted 4 lines

Actual filling buffer is done in the profile handlers, so I think ftrace_perf_buf_prepare() may be better :-) ftrace_perf_buf_submit is good to me:-)

OK, i'll rename those functions and add __kprobe for ftrace_perf_buf_prepare(), i guess i can add you Acked-by :-)

Quoted 7 lines · Xiao Guangrong

Quoted 4 lines

Actual filling buffer is done in the profile handlers, so I think ftrace_perf_buf_prepare() may be better :-) ftrace_perf_buf_submit is good to me:-)

OK, i'll rename those functions and add __kprobe for ftrace_perf_buf_prepare(), i guess i can add you Acked-by :-)

You can add mine yeah (couldn't tell for Masami).

Introduce ftrace_profile_buf_begin() and ftrace_profile_buf_end() to operate event profile buffer, clean up redundant code

Signed-off-by Xiao Guangrong <[email protected]>

---

It only disable preemption in perf_swevent_get_recursion_context() it can't avoid race of hard-irq and NMI

In this patch, we use atomic operation to avoid it and reduce cpu_ctx->recursion size, it also make this patch no need diable preemption

Quoted 6 lines · Xiao Guangrong

It only disable preemption in perf_swevent_get_recursion_context() it can't avoid race of hard-irq and NMI

In this patch, we use atomic operation to avoid it and reduce cpu_ctx->recursion size, it also make this patch no need diable preemption

Uhm why?

This patch looks terminally broken

Hi Peter,

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Xiao Guangrong

It only disable preemption in perf_swevent_get_recursion_context() it can't avoid race of hard-irq and NMI

In this patch, we use atomic operation to avoid it and reduce cpu_ctx->recursion size, it also make this patch no need diable preemption

Quoted 3 lines

Uhm why?

This patch looks terminally broken

Please see my explanation in another mail:
http://lkml.org/lkml/2010/1/18/501

It only disable preemption in perf_swevent_get_recursion_context() it can't avoid race of hard-irq and NMI

In this patch, we use atomic operation to avoid it and reduce cpu_ctx->recursion size, it also make this patch no need diable preemption

Signed-off-by Xiao Guangrong <[email protected]>

int perf_swevent_get_recursion_context(void)
{

kernel/perf_event.c +1 -1

-	struct perf_cpu_context *cpuctx = &get_cpu_var(perf_cpu_context);
+	struct perf_cpu_context *cpuctx = &__get_cpu_var(perf_cpu_context);
 	int rctx;
 
 	if (in_nmi())

Quoted 7 lines · Xiao Guangrong

It only disable preemption in perf_swevent_get_recursion_context() it can't avoid race of hard-irq and NMI

In this patch, we use atomic operation to avoid it and reduce cpu_ctx->recursion size, it also make this patch no need diable preemption

Quoted 1 line

Signed-off-by Xiao Guangrong <[email protected]>

I don't understand what is racy in what we have currently.

Quoted 13 lines

int perf_swevent_get_recursion_context(void)
{

kernel/perf_event.c +1 -1

-	struct perf_cpu_context *cpuctx = &get_cpu_var(perf_cpu_context);
+	struct perf_cpu_context *cpuctx = &__get_cpu_var(perf_cpu_context);
 	int rctx;
 
 	if (in_nmi())

@@ -3933,13 +3933,8 @@ int perf_swevent_get_recursion_context(void)
 	else
 		rctx = 0;
 

kernel/perf_event.c int perf_swevent_get_recursion_context(void) · line 3937

Quoted 4 lines

kernel/perf_event.c +1 -2

-	if (cpuctx->recursion[rctx]) {
-		put_cpu_var(perf_cpu_context);
+	if (test_and_set_bit(rctx, &cpuctx->recursion))
 		return -1;

This looks broken. We don't call back perf_swevent_put_recursion_context in fail case, so the bit won't ever be cleared once we recurse.

It's because hard-irq(we can handle interruption with interruption enabled) and NMI are nested, for example:

int perf_swevent_get_recursion_context(void)

{
        ......
        if (cpuctx->recursion[rctx]) {
                put_cpu_var(perf_cpu_context);
                return -1;
        }

        /*
         * Another interruption handler/NMI will re-enter there if it
         * happed, it make the recursion value chaotic
         */
        cpuctx->recursion[rctx]++;
        ......
} 

Quoted 8 lines · Frederic Weisbecker

Quoted 7 lines · Xiao Guangrong

It only disable preemption in perf_swevent_get_recursion_context() it can't avoid race of hard-irq and NMI

In this patch, we use atomic operation to avoid it and reduce cpu_ctx->recursion size, it also make this patch no need diable preemption

Quoted 6 lines

Quoted 1 line

Signed-off-by Xiao Guangrong <[email protected]>

I don't understand what is racy in what we have currently.

It's because hard-irq(we can handle interruption with interruption enabled) and NMI are nested, for example:

int perf_swevent_get_recursion_context(void)

{
	......
	if (cpuctx->recursion[rctx]) {
		put_cpu_var(perf_cpu_context);
		return -1;
	}

	/*
	 * Another interruption handler/NMI will re-enter there if it
	 * happed, it make the recursion value chaotic
	 */
	cpuctx->recursion[rctx]++;
	......

Quoted 3 lines

This looks broken. We don't call back perf_swevent_put_recursion_context in fail case, so the bit won't ever be cleared once we recurse.

Um, i think we can't clear the bit in this fail case, consider below sequence:

path A: path B

set bit but find the bit already set

 atomic set bit                                 |
    |                                           |
    V                                           |
 handle SW event                                | 
    |                                           V

V exit and not clear the bit
atomic clear bit

After A and B, the bit is still zero

Right? :-)

I still don't understand the problem.

It's not like a fight between different cpus, it's a local per cpu fight.

NMIs can't nest other NMIs but hardirq can nest another hardirqs, we don't care much about these though.
So let's imagine the following sequence, a fight between nested hardirqs:

cpuctx->recursion[irq] initially = 0

Interrupt (level 0):

       if (cpuctx->recursion[rctx]) {
               put_cpu_var(perf_cpu_context);
               return -1;
       }

Interrupt (level 1):

	cpuctx->recursion[rctx]++; // = 1

	...
	do something
	...
	cpuctx->recursion[rctx]--; // = 0

End Interrupt (level 1)

	cpuctx->recursion[rctx]++; // = 1

        ...
        do something
        ...
        cpuctx->recursion[rctx]--; // = 0

End interrupt (level 0)

Another sequence could be Interrupt level 0 has already incremented recursion and we are interrupted by irq level 1 which then won't be able to get the recursion context. But that's not a big deal I think.