live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing
@ 2021-10-12  5:39 王贇
  2021-10-12  5:40 ` [PATCH 1/2] ftrace: disable preemption on the testing of recursion 王贇
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: 王贇 @ 2021-10-12  5:39 UTC (permalink / raw)
  To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

The testing show that perf_ftrace_function_call() are using
smp_processor_id() with preemption enabled, all the checking
on CPU could be wrong after preemption, PATCH 1/2 will fix
that.

Besides, as Peter point out, the testing of recursion within
the section between ftrace_test_recursion_trylock()/_unlock()
pair also need the preemption disabled as the documentation
explained, PATCH 2/2 will make sure on that.

Michael Wang (2):
  ftrace: disable preemption on the testing of recursion
  ftrace: prevent preemption in perf_ftrace_function_call()

 arch/csky/kernel/probes/ftrace.c     |  2 --
 arch/parisc/kernel/ftrace.c          |  2 --
 arch/powerpc/kernel/kprobes-ftrace.c |  2 --
 arch/riscv/kernel/probes/ftrace.c    |  2 --
 arch/x86/kernel/kprobes/ftrace.c     |  2 --
 include/linux/trace_recursion.h      | 10 +++++++++-
 kernel/livepatch/patch.c             |  6 ------
 kernel/trace/trace_event_perf.c      | 17 +++++++++++++----
 kernel/trace/trace_functions.c       |  5 -----
 9 files changed, 22 insertions(+), 26 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12  5:39 [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
@ 2021-10-12  5:40 ` 王贇
  2021-10-12 12:17   ` Steven Rostedt
                     ` (2 more replies)
  2021-10-12  5:40 ` [PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call() 王贇
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 24+ messages in thread
From: 王贇 @ 2021-10-12  5:40 UTC (permalink / raw)
  To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

As the documentation explained, ftrace_test_recursion_trylock()
and ftrace_test_recursion_unlock() were supposed to disable and
enable preemption properly, however currently this work is done
outside of the function, which could be missing by mistake.

This path will make sure the preemption will be disabled when
trylock() succeed, and the unlock() will enable preemption when
the the testing of recursion are finished.

Reported-by: Abaci <abaci@linux.alibaba.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
 arch/csky/kernel/probes/ftrace.c     |  2 --
 arch/parisc/kernel/ftrace.c          |  2 --
 arch/powerpc/kernel/kprobes-ftrace.c |  2 --
 arch/riscv/kernel/probes/ftrace.c    |  2 --
 arch/x86/kernel/kprobes/ftrace.c     |  2 --
 include/linux/trace_recursion.h      | 10 +++++++++-
 kernel/livepatch/patch.c             |  6 ------
 kernel/trace/trace_functions.c       |  5 -----
 8 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c
index ef2bb9b..dff7921 100644
--- a/arch/csky/kernel/probes/ftrace.c
+++ b/arch/csky/kernel/probes/ftrace.c
@@ -24,7 +24,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		return;

 	regs = ftrace_get_regs(fregs);
-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (!p) {
 		p = get_kprobe((kprobe_opcode_t *)(ip - MCOUNT_INSN_SIZE));
@@ -64,7 +63,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
index 0a1e75a..3543496 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -216,7 +216,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		return;

 	regs = ftrace_get_regs(fregs);
-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))
 		goto out;
@@ -245,7 +244,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	}
 	__this_cpu_write(current_kprobe, NULL);
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c
index 7154d58..072ebe7 100644
--- a/arch/powerpc/kernel/kprobes-ftrace.c
+++ b/arch/powerpc/kernel/kprobes-ftrace.c
@@ -26,7 +26,6 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
 		return;

 	regs = ftrace_get_regs(fregs);
-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)nip);
 	if (unlikely(!p) || kprobe_disabled(p))
 		goto out;
@@ -61,7 +60,6 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/riscv/kernel/probes/ftrace.c b/arch/riscv/kernel/probes/ftrace.c
index aab85a8..7142ec4 100644
--- a/arch/riscv/kernel/probes/ftrace.c
+++ b/arch/riscv/kernel/probes/ftrace.c
@@ -15,7 +15,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (bit < 0)
 		return;

-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))
 		goto out;
@@ -52,7 +51,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 596de2f..dd2ec14 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -25,7 +25,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (bit < 0)
 		return;

-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))
 		goto out;
@@ -59,7 +58,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
index a9f9c57..805f9c4 100644
--- a/include/linux/trace_recursion.h
+++ b/include/linux/trace_recursion.h
@@ -214,7 +214,14 @@ static __always_inline void trace_clear_recursion(int bit)
 static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
 							 unsigned long parent_ip)
 {
-	return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
+	int bit;
+
+	preempt_disable_notrace();
+	bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
+	if (bit < 0)
+		preempt_enable_notrace();
+
+	return bit;
 }

 /**
@@ -226,6 +233,7 @@ static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
 static __always_inline void ftrace_test_recursion_unlock(int bit)
 {
 	trace_clear_recursion(bit);
+	preempt_enable_notrace();
 }

 #endif /* CONFIG_TRACING */
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index e8029ae..6e66ccd 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	bit = ftrace_test_recursion_trylock(ip, parent_ip);
 	if (WARN_ON_ONCE(bit < 0))
 		return;
-	/*
-	 * A variant of synchronize_rcu() is used to allow patching functions
-	 * where RCU is not watching, see klp_synchronize_transition().
-	 */
-	preempt_disable_notrace();

 	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
 				      stack_node);
@@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	klp_arch_set_pc(fregs, (unsigned long)func->new_func);

 unlock:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 1f0e63f..9f1bfbe 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -186,7 +186,6 @@ static void function_trace_start(struct trace_array *tr)
 		return;

 	trace_ctx = tracing_gen_ctx();
-	preempt_disable_notrace();

 	cpu = smp_processor_id();
 	data = per_cpu_ptr(tr->array_buffer.data, cpu);
@@ -194,7 +193,6 @@ static void function_trace_start(struct trace_array *tr)
 		trace_function(tr, ip, parent_ip, trace_ctx);

 	ftrace_test_recursion_unlock(bit);
-	preempt_enable_notrace();
 }

 #ifdef CONFIG_UNWINDER_ORC
@@ -298,8 +296,6 @@ static inline void process_repeats(struct trace_array *tr,
 	if (bit < 0)
 		return;

-	preempt_disable_notrace();
-
 	cpu = smp_processor_id();
 	data = per_cpu_ptr(tr->array_buffer.data, cpu);
 	if (atomic_read(&data->disabled))
@@ -324,7 +320,6 @@ static inline void process_repeats(struct trace_array *tr,

 out:
 	ftrace_test_recursion_unlock(bit);
-	preempt_enable_notrace();
 }

 static void
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call()
  2021-10-12  5:39 [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
  2021-10-12  5:40 ` [PATCH 1/2] ftrace: disable preemption on the testing of recursion 王贇
@ 2021-10-12  5:40 ` 王贇
  2021-10-12 11:20   ` Peter Zijlstra
  2021-10-12  5:41 ` [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: 王贇 @ 2021-10-12  5:40 UTC (permalink / raw)
  To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

With CONFIG_DEBUG_PREEMPT we observed reports like:

  BUG: using smp_processor_id() in preemptible
  caller is perf_ftrace_function_call+0x6f/0x2e0
  CPU: 1 PID: 680 Comm: a.out Not tainted
  Call Trace:
   <TASK>
   dump_stack_lvl+0x8d/0xcf
   check_preemption_disabled+0x104/0x110
   ? optimize_nops.isra.7+0x230/0x230
   ? text_poke_bp_batch+0x9f/0x310
   perf_ftrace_function_call+0x6f/0x2e0
   ...
   __text_poke+0x5/0x620
   text_poke_bp_batch+0x9f/0x310

This telling us the CPU could be changed after task is preempted, and
the checking on CPU before preemption will be invalid.

This patch just turn off preemption in perf_ftrace_function_call()
to prevent CPU changing.

CC: Steven Rostedt <rostedt@goodmis.org>
Reported-by: Abaci <abaci@linux.alibaba.com>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
 kernel/trace/trace_event_perf.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 6aed10e..33c2f76 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -441,12 +441,19 @@ void perf_trace_buf_update(void *record, u16 type)
 	if (!rcu_is_watching())
 		return;

+	/*
+	 * Prevent CPU changing from now on. rcu must
+	 * be in watching if the task was migrated and
+	 * scheduled.
+	 */
+	preempt_disable_notrace();
+
 	if ((unsigned long)ops->private != smp_processor_id())
-		return;
+		goto out;

 	bit = ftrace_test_recursion_trylock(ip, parent_ip);
 	if (bit < 0)
-		return;
+		goto out;

 	event = container_of(ops, struct perf_event, ftrace_ops);

@@ -468,16 +475,18 @@ void perf_trace_buf_update(void *record, u16 type)

 	entry = perf_trace_buf_alloc(ENTRY_SIZE, NULL, &rctx);
 	if (!entry)
-		goto out;
+		goto unlock;

 	entry->ip = ip;
 	entry->parent_ip = parent_ip;
 	perf_trace_buf_submit(entry, ENTRY_SIZE, rctx, TRACE_FN,
 			      1, &regs, &head, NULL);

-out:
+unlock:
 	ftrace_test_recursion_unlock(bit);
 #undef ENTRY_SIZE
+out:
+	preempt_enable_notrace();
 }

 static int perf_ftrace_function_register(struct perf_event *event)
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing
  2021-10-12  5:39 [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
  2021-10-12  5:40 ` [PATCH 1/2] ftrace: disable preemption on the testing of recursion 王贇
  2021-10-12  5:40 ` [PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call() 王贇
@ 2021-10-12  5:41 ` 王贇
  2021-10-13  3:16 ` [PATCH v2 0/2] fix & prevent the missing preemption disabling 王贇
  2021-10-26 23:48 ` [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing Palmer Dabbelt
  4 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-12  5:41 UTC (permalink / raw)
  To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching



On 2021/10/12 下午1:39, 王贇 wrote:
> The testing show that perf_ftrace_function_call() are using
> smp_processor_id() with preemption enabled, all the checking
> on CPU could be wrong after preemption, PATCH 1/2 will fix
> that.

2/2 actually.

> 
> Besides, as Peter point out, the testing of recursion within
> the section between ftrace_test_recursion_trylock()/_unlock()
> pair also need the preemption disabled as the documentation
> explained, PATCH 2/2 will make sure on that.

1/2 actually...

Regards,
Michael Wang

> 
> Michael Wang (2):
>   ftrace: disable preemption on the testing of recursion
>   ftrace: prevent preemption in perf_ftrace_function_call()
> 
>  arch/csky/kernel/probes/ftrace.c     |  2 --
>  arch/parisc/kernel/ftrace.c          |  2 --
>  arch/powerpc/kernel/kprobes-ftrace.c |  2 --
>  arch/riscv/kernel/probes/ftrace.c    |  2 --
>  arch/x86/kernel/kprobes/ftrace.c     |  2 --
>  include/linux/trace_recursion.h      | 10 +++++++++-
>  kernel/livepatch/patch.c             |  6 ------
>  kernel/trace/trace_event_perf.c      | 17 +++++++++++++----
>  kernel/trace/trace_functions.c       |  5 -----
>  9 files changed, 22 insertions(+), 26 deletions(-)
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call()
  2021-10-12  5:40 ` [PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call() 王贇
@ 2021-10-12 11:20   ` Peter Zijlstra
  2021-10-13  1:45     ` 王贇
  0 siblings, 1 reply; 24+ messages in thread
From: Peter Zijlstra @ 2021-10-12 11:20 UTC (permalink / raw)
  To: 王贇
  Cc: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu, Nicholas Piggin,
	Jisheng Zhang, linux-csky, linux-kernel, linux-parisc,
	linuxppc-dev, linux-riscv, live-patching

On Tue, Oct 12, 2021 at 01:40:31PM +0800, 王贇 wrote:

> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index 6aed10e..33c2f76 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -441,12 +441,19 @@ void perf_trace_buf_update(void *record, u16 type)
>  	if (!rcu_is_watching())
>  		return;
> 
> +	/*
> +	 * Prevent CPU changing from now on. rcu must
> +	 * be in watching if the task was migrated and
> +	 * scheduled.
> +	 */
> +	preempt_disable_notrace();
> +
>  	if ((unsigned long)ops->private != smp_processor_id())
> -		return;
> +		goto out;
> 
>  	bit = ftrace_test_recursion_trylock(ip, parent_ip);
>  	if (bit < 0)
> -		return;
> +		goto out;
> 
>  	event = container_of(ops, struct perf_event, ftrace_ops);
> 

This seems rather daft, wouldn't it be easier to just put that check
under the recursion thing?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12  5:40 ` [PATCH 1/2] ftrace: disable preemption on the testing of recursion 王贇
@ 2021-10-12 12:17   ` Steven Rostedt
  2021-10-13  1:46     ` 王贇
  2021-10-12 12:24   ` Miroslav Benes
  2021-10-12 12:43   ` Steven Rostedt
  2 siblings, 1 reply; 24+ messages in thread
From: Steven Rostedt @ 2021-10-12 12:17 UTC (permalink / raw)
  To: 王贇
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

On Tue, 12 Oct 2021 13:40:08 +0800
王贇 <yun.wang@linux.alibaba.com> wrote:

> @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>  	bit = ftrace_test_recursion_trylock(ip, parent_ip);
>  	if (WARN_ON_ONCE(bit < 0))
>  		return;
> -	/*
> -	 * A variant of synchronize_rcu() is used to allow patching functions
> -	 * where RCU is not watching, see klp_synchronize_transition().
> -	 */

I have to take a deeper look at this patch set, but do not remove this
comment, as it explains the protection here, that is not obvious with the
changes you made.

-- Steve


> -	preempt_disable_notrace();
> 
>  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
>  				      stack_node);

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12  5:40 ` [PATCH 1/2] ftrace: disable preemption on the testing of recursion 王贇
  2021-10-12 12:17   ` Steven Rostedt
@ 2021-10-12 12:24   ` Miroslav Benes
  2021-10-12 12:29     ` Steven Rostedt
  2021-10-13  1:50     ` 王贇
  2021-10-12 12:43   ` Steven Rostedt
  2 siblings, 2 replies; 24+ messages in thread
From: Miroslav Benes @ 2021-10-12 12:24 UTC (permalink / raw)
  To: 王贇
  Cc: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

> diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
> index a9f9c57..805f9c4 100644
> --- a/include/linux/trace_recursion.h
> +++ b/include/linux/trace_recursion.h
> @@ -214,7 +214,14 @@ static __always_inline void trace_clear_recursion(int bit)
>  static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
>  							 unsigned long parent_ip)
>  {
> -	return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> +	int bit;
> +
> +	preempt_disable_notrace();
> +	bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> +	if (bit < 0)
> +		preempt_enable_notrace();
> +
> +	return bit;
>  }
> 
>  /**
> @@ -226,6 +233,7 @@ static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
>  static __always_inline void ftrace_test_recursion_unlock(int bit)
>  {
>  	trace_clear_recursion(bit);
> +	preempt_enable_notrace();
>  }
> 
>  #endif /* CONFIG_TRACING */
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index e8029ae..6e66ccd 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>  	bit = ftrace_test_recursion_trylock(ip, parent_ip);
>  	if (WARN_ON_ONCE(bit < 0))
>  		return;
> -	/*
> -	 * A variant of synchronize_rcu() is used to allow patching functions
> -	 * where RCU is not watching, see klp_synchronize_transition().
> -	 */
> -	preempt_disable_notrace();
> 
>  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
>  				      stack_node);
> @@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>  	klp_arch_set_pc(fregs, (unsigned long)func->new_func);
> 
>  unlock:
> -	preempt_enable_notrace();
>  	ftrace_test_recursion_unlock(bit);
>  }

I don't like this change much. We have preempt_disable there not because 
of ftrace_test_recursion, but because of RCU. ftrace_test_recursion was 
added later. Yes, it would work with the change, but it would also hide 
things which should not be hidden in my opinion.

Miroslav

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12 12:24   ` Miroslav Benes
@ 2021-10-12 12:29     ` Steven Rostedt
  2021-10-13  1:52       ` 王贇
  2021-10-13  1:50     ` 王贇
  1 sibling, 1 reply; 24+ messages in thread
From: Steven Rostedt @ 2021-10-12 12:29 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: 王贇,
	Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Petr Mladek, Joe Lawrence, Colin Ian King,
	Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

On Tue, 12 Oct 2021 14:24:43 +0200 (CEST)
Miroslav Benes <mbenes@suse.cz> wrote:

> > +++ b/kernel/livepatch/patch.c
> > @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> >  	bit = ftrace_test_recursion_trylock(ip, parent_ip);
> >  	if (WARN_ON_ONCE(bit < 0))
> >  		return;
> > -	/*
> > -	 * A variant of synchronize_rcu() is used to allow patching functions
> > -	 * where RCU is not watching, see klp_synchronize_transition().
> > -	 */
> > -	preempt_disable_notrace();
> > 
> >  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
> >  				      stack_node);
> > @@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> >  	klp_arch_set_pc(fregs, (unsigned long)func->new_func);
> > 
> >  unlock:
> > -	preempt_enable_notrace();
> >  	ftrace_test_recursion_unlock(bit);
> >  }  
> 
> I don't like this change much. We have preempt_disable there not because 
> of ftrace_test_recursion, but because of RCU. ftrace_test_recursion was 
> added later. Yes, it would work with the change, but it would also hide 
> things which should not be hidden in my opinion.

Agreed, but I believe the change is fine, but requires a nice comment to
explain what you said above.

Thus, before the "ftrace_test_recursion_trylock()" we need:

	/*
	 * The ftrace_test_recursion_trylock() will disable preemption,
	 * which is required for the variant of synchronize_rcu() that is
	 * used to allow patching functions where RCU is not watching.
	 * See klp_synchronize_transition() for more details.
	 */

-- Steve

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12  5:40 ` [PATCH 1/2] ftrace: disable preemption on the testing of recursion 王贇
  2021-10-12 12:17   ` Steven Rostedt
  2021-10-12 12:24   ` Miroslav Benes
@ 2021-10-12 12:43   ` Steven Rostedt
  2021-10-13  2:04     ` 王贇
  2 siblings, 1 reply; 24+ messages in thread
From: Steven Rostedt @ 2021-10-12 12:43 UTC (permalink / raw)
  To: 王贇
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

On Tue, 12 Oct 2021 13:40:08 +0800
王贇 <yun.wang@linux.alibaba.com> wrote:

> --- a/include/linux/trace_recursion.h
> +++ b/include/linux/trace_recursion.h
> @@ -214,7 +214,14 @@ static __always_inline void trace_clear_recursion(int bit)
>  static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
>  							 unsigned long parent_ip)
>  {
> -	return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> +	int bit;
> +
> +	preempt_disable_notrace();

The recursion test does not require preemption disabled, it uses the task
struct, not per_cpu variables, so you should not disable it before the test.

	bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
	if (bit >= 0)
		preempt_disable_notrace();

And if the bit is zero, it means a recursion check was already done by
another caller (ftrace handler does the check, followed by calling perf),
and you really don't even need to disable preemption in that case.

	if (bit > 0)
		preempt_disable_notrace();

And on the unlock, have:

 static __always_inline void ftrace_test_recursion_unlock(int bit)
 {
	if (bit)
		preempt_enable_notrace();
 	trace_clear_recursion(bit);
 }

But maybe that's over optimizing ;-)

-- Steve


> +	bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> +	if (bit < 0)
> +		preempt_enable_notrace();
> +
> +	return bit;
>  }


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call()
  2021-10-12 11:20   ` Peter Zijlstra
@ 2021-10-13  1:45     ` 王贇
  0 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  1:45 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu, Nicholas Piggin,
	Jisheng Zhang, linux-csky, linux-kernel, linux-parisc,
	linuxppc-dev, linux-riscv, live-patching



On 2021/10/12 下午7:20, Peter Zijlstra wrote:
> On Tue, Oct 12, 2021 at 01:40:31PM +0800, 王贇 wrote:
> 
>> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
>> index 6aed10e..33c2f76 100644
>> --- a/kernel/trace/trace_event_perf.c
>> +++ b/kernel/trace/trace_event_perf.c
>> @@ -441,12 +441,19 @@ void perf_trace_buf_update(void *record, u16 type)
>>  	if (!rcu_is_watching())
>>  		return;
>>
>> +	/*
>> +	 * Prevent CPU changing from now on. rcu must
>> +	 * be in watching if the task was migrated and
>> +	 * scheduled.
>> +	 */
>> +	preempt_disable_notrace();
>> +
>>  	if ((unsigned long)ops->private != smp_processor_id())
>> -		return;
>> +		goto out;
>>
>>  	bit = ftrace_test_recursion_trylock(ip, parent_ip);
>>  	if (bit < 0)
>> -		return;
>> +		goto out;
>>
>>  	event = container_of(ops, struct perf_event, ftrace_ops);
>>
> 
> This seems rather daft, wouldn't it be easier to just put that check
> under the recursion thing?

In case if the condition matched, extra lock/unlock will be introduced,
but I guess that's acceptable since this seems unlikely to happen :-P

Will move the check in v2.

Regards,
Michael Wang

> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12 12:17   ` Steven Rostedt
@ 2021-10-13  1:46     ` 王贇
  0 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  1:46 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching



On 2021/10/12 下午8:17, Steven Rostedt wrote:
> On Tue, 12 Oct 2021 13:40:08 +0800
> 王贇 <yun.wang@linux.alibaba.com> wrote:
> 
>> @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>>  	bit = ftrace_test_recursion_trylock(ip, parent_ip);
>>  	if (WARN_ON_ONCE(bit < 0))
>>  		return;
>> -	/*
>> -	 * A variant of synchronize_rcu() is used to allow patching functions
>> -	 * where RCU is not watching, see klp_synchronize_transition().
>> -	 */
> 
> I have to take a deeper look at this patch set, but do not remove this
> comment, as it explains the protection here, that is not obvious with the
> changes you made.

Will keep that in v2.

Regards,
Michael Wang

> 
> -- Steve
> 
> 
>> -	preempt_disable_notrace();
>>
>>  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
>>  				      stack_node);

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12 12:24   ` Miroslav Benes
  2021-10-12 12:29     ` Steven Rostedt
@ 2021-10-13  1:50     ` 王贇
  2021-10-13  2:27       ` Steven Rostedt
  1 sibling, 1 reply; 24+ messages in thread
From: 王贇 @ 2021-10-13  1:50 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching



On 2021/10/12 下午8:24, Miroslav Benes wrote:
[snip]
>>
>>  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
>>  				      stack_node);
>> @@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>>  	klp_arch_set_pc(fregs, (unsigned long)func->new_func);
>>
>>  unlock:
>> -	preempt_enable_notrace();
>>  	ftrace_test_recursion_unlock(bit);
>>  }
> 
> I don't like this change much. We have preempt_disable there not because 
> of ftrace_test_recursion, but because of RCU. ftrace_test_recursion was 
> added later. Yes, it would work with the change, but it would also hide 
> things which should not be hidden in my opinion.

Not very sure about the backgroup stories, but just found this in
'Documentation/trace/ftrace-uses.rst':

  Note, on success,
  ftrace_test_recursion_trylock() will disable preemption, and the
  ftrace_test_recursion_unlock() will enable it again (if it was previously
  enabled).

Seems like this lock pair was supposed to take care the preemtion itself?

Regards,
Michael Wang

> 
> Miroslav
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12 12:29     ` Steven Rostedt
@ 2021-10-13  1:52       ` 王贇
  0 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  1:52 UTC (permalink / raw)
  To: Steven Rostedt, Miroslav Benes
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Petr Mladek, Joe Lawrence, Colin Ian King,
	Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching



On 2021/10/12 下午8:29, Steven Rostedt wrote:
> On Tue, 12 Oct 2021 14:24:43 +0200 (CEST)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
>>> +++ b/kernel/livepatch/patch.c
>>> @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>>>  	bit = ftrace_test_recursion_trylock(ip, parent_ip);
>>>  	if (WARN_ON_ONCE(bit < 0))
>>>  		return;
>>> -	/*
>>> -	 * A variant of synchronize_rcu() is used to allow patching functions
>>> -	 * where RCU is not watching, see klp_synchronize_transition().
>>> -	 */
>>> -	preempt_disable_notrace();
>>>
>>>  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
>>>  				      stack_node);
>>> @@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>>>  	klp_arch_set_pc(fregs, (unsigned long)func->new_func);
>>>
>>>  unlock:
>>> -	preempt_enable_notrace();
>>>  	ftrace_test_recursion_unlock(bit);
>>>  }  
>>
>> I don't like this change much. We have preempt_disable there not because 
>> of ftrace_test_recursion, but because of RCU. ftrace_test_recursion was 
>> added later. Yes, it would work with the change, but it would also hide 
>> things which should not be hidden in my opinion.
> 
> Agreed, but I believe the change is fine, but requires a nice comment to
> explain what you said above.
> 
> Thus, before the "ftrace_test_recursion_trylock()" we need:
> 
> 	/*
> 	 * The ftrace_test_recursion_trylock() will disable preemption,
> 	 * which is required for the variant of synchronize_rcu() that is
> 	 * used to allow patching functions where RCU is not watching.
> 	 * See klp_synchronize_transition() for more details.
> 	 */

Will be in v2 too :-)

Regards,
Michael Wang

> 
> -- Steve
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-12 12:43   ` Steven Rostedt
@ 2021-10-13  2:04     ` 王贇
  2021-10-13  2:30       ` Steven Rostedt
  0 siblings, 1 reply; 24+ messages in thread
From: 王贇 @ 2021-10-13  2:04 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching



On 2021/10/12 下午8:43, Steven Rostedt wrote:
> On Tue, 12 Oct 2021 13:40:08 +0800
> 王贇 <yun.wang@linux.alibaba.com> wrote:
> 
>> --- a/include/linux/trace_recursion.h
>> +++ b/include/linux/trace_recursion.h
>> @@ -214,7 +214,14 @@ static __always_inline void trace_clear_recursion(int bit)
>>  static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
>>  							 unsigned long parent_ip)
>>  {
>> -	return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
>> +	int bit;
>> +
>> +	preempt_disable_notrace();
> 
> The recursion test does not require preemption disabled, it uses the task
> struct, not per_cpu variables, so you should not disable it before the test.
> 
> 	bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> 	if (bit >= 0)
> 		preempt_disable_notrace();
> 
> And if the bit is zero, it means a recursion check was already done by
> another caller (ftrace handler does the check, followed by calling perf),
> and you really don't even need to disable preemption in that case.
> 
> 	if (bit > 0)
> 		preempt_disable_notrace();
> 
> And on the unlock, have:
> 
>  static __always_inline void ftrace_test_recursion_unlock(int bit)
>  {
> 	if (bit)
> 		preempt_enable_notrace();
>  	trace_clear_recursion(bit);
>  }
> 
> But maybe that's over optimizing ;-)

I see, while the user can still check smp_processor_id() after trylock
return bit 0...

I guess Peter's point at very beginning is to prevent such cases, since
kernel for production will not have preemption debug on, and such issue
won't get report but could cause trouble which really hard to trace down
, way to eliminate such issue once for all sounds attractive, isn't it?

Regards,
Michael Wang

> 
> -- Steve
> 
> 
>> +	bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
>> +	if (bit < 0)
>> +		preempt_enable_notrace();
>> +
>> +	return bit;
>>  }

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-13  1:50     ` 王贇
@ 2021-10-13  2:27       ` Steven Rostedt
  2021-10-13  2:36         ` 王贇
  0 siblings, 1 reply; 24+ messages in thread
From: Steven Rostedt @ 2021-10-13  2:27 UTC (permalink / raw)
  To: 王贇
  Cc: Miroslav Benes, Guo Ren, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

On Wed, 13 Oct 2021 09:50:17 +0800
王贇 <yun.wang@linux.alibaba.com> wrote:

> >> -	preempt_enable_notrace();
> >>  	ftrace_test_recursion_unlock(bit);
> >>  }  
> > 
> > I don't like this change much. We have preempt_disable there not because 
> > of ftrace_test_recursion, but because of RCU. ftrace_test_recursion was 
> > added later. Yes, it would work with the change, but it would also hide 
> > things which should not be hidden in my opinion.  
> 
> Not very sure about the backgroup stories, but just found this in
> 'Documentation/trace/ftrace-uses.rst':
> 
>   Note, on success,
>   ftrace_test_recursion_trylock() will disable preemption, and the
>   ftrace_test_recursion_unlock() will enable it again (if it was previously
>   enabled).

Right that part is to be fixed by what you are adding here.

The point that Miroslav is complaining about is that the preemption
disabling is special in this case, and not just from the recursion
point of view, which is why the comment is still required.

-- Steve


> 
> Seems like this lock pair was supposed to take care the preemtion itself?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-13  2:04     ` 王贇
@ 2021-10-13  2:30       ` Steven Rostedt
  2021-10-13  2:38         ` 王贇
  0 siblings, 1 reply; 24+ messages in thread
From: Steven Rostedt @ 2021-10-13  2:30 UTC (permalink / raw)
  To: 王贇
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

On Wed, 13 Oct 2021 10:04:52 +0800
王贇 <yun.wang@linux.alibaba.com> wrote:

> I see, while the user can still check smp_processor_id() after trylock
> return bit 0...

But preemption would have already been disabled. That's because a bit 0
means that a recursion check has already been made by a previous
caller and this one is nested, thus preemption is already disabled.
If bit is 0, then preemption had better be disabled as well.

-- Steve

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-13  2:27       ` Steven Rostedt
@ 2021-10-13  2:36         ` 王贇
  0 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  2:36 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Miroslav Benes, Guo Ren, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching



On 2021/10/13 上午10:27, Steven Rostedt wrote:
> On Wed, 13 Oct 2021 09:50:17 +0800
> 王贇 <yun.wang@linux.alibaba.com> wrote:
> 
>>>> -	preempt_enable_notrace();
>>>>  	ftrace_test_recursion_unlock(bit);
>>>>  }  
>>>
>>> I don't like this change much. We have preempt_disable there not because 
>>> of ftrace_test_recursion, but because of RCU. ftrace_test_recursion was 
>>> added later. Yes, it would work with the change, but it would also hide 
>>> things which should not be hidden in my opinion.  
>>
>> Not very sure about the backgroup stories, but just found this in
>> 'Documentation/trace/ftrace-uses.rst':
>>
>>   Note, on success,
>>   ftrace_test_recursion_trylock() will disable preemption, and the
>>   ftrace_test_recursion_unlock() will enable it again (if it was previously
>>   enabled).
> 
> Right that part is to be fixed by what you are adding here.
> 
> The point that Miroslav is complaining about is that the preemption
> disabling is special in this case, and not just from the recursion
> point of view, which is why the comment is still required.

My bad... the title do confusing people, will rewrite it.

Regards,
Michael Wang

> 
> -- Steve
> 
> 
>>
>> Seems like this lock pair was supposed to take care the preemtion itself?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion
  2021-10-13  2:30       ` Steven Rostedt
@ 2021-10-13  2:38         ` 王贇
  0 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  2:38 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching



On 2021/10/13 上午10:30, Steven Rostedt wrote:
> On Wed, 13 Oct 2021 10:04:52 +0800
> 王贇 <yun.wang@linux.alibaba.com> wrote:
> 
>> I see, while the user can still check smp_processor_id() after trylock
>> return bit 0...
> 
> But preemption would have already been disabled. That's because a bit 0
> means that a recursion check has already been made by a previous
> caller and this one is nested, thus preemption is already disabled.
> If bit is 0, then preemption had better be disabled as well.

Thanks for the explain, now I get your point :-)

Let's make bit 0 an exemption then.

Regards,
Michael Wang

> 
> -- Steve
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 0/2] fix & prevent the missing preemption disabling
  2021-10-12  5:39 [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
                   ` (2 preceding siblings ...)
  2021-10-12  5:41 ` [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
@ 2021-10-13  3:16 ` 王贇
  2021-10-13  3:17   ` [PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock() 王贇
                     ` (2 more replies)
  2021-10-26 23:48 ` [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing Palmer Dabbelt
  4 siblings, 3 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  3:16 UTC (permalink / raw)
  To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

The testing show that perf_ftrace_function_call() are using smp_processor_id()
with preemption enabled, all the checking on CPU could be wrong after preemption.

As Peter point out, the section between ftrace_test_recursion_trylock/unlock()
pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst'
explained, but currently the work is done outside of the helpers.

Patch 1/2 will make sure preemption disabled after trylock() succeed,
patch 2/2 will do smp_processor_id() checking after trylock to address the
issue.

Michael Wang (2):
  ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
  ftrace: do CPU checking after preemption disabled

 arch/csky/kernel/probes/ftrace.c     |  2 --
 arch/parisc/kernel/ftrace.c          |  2 --
 arch/powerpc/kernel/kprobes-ftrace.c |  2 --
 arch/riscv/kernel/probes/ftrace.c    |  2 --
 arch/x86/kernel/kprobes/ftrace.c     |  2 --
 include/linux/trace_recursion.h      | 22 +++++++++++++++++++++-
 kernel/livepatch/patch.c             |  6 ------
 kernel/trace/trace_event_perf.c      |  6 +++---
 kernel/trace/trace_functions.c       |  5 -----
 9 files changed, 24 insertions(+), 25 deletions(-)

-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
  2021-10-13  3:16 ` [PATCH v2 0/2] fix & prevent the missing preemption disabling 王贇
@ 2021-10-13  3:17   ` 王贇
  2021-10-13  3:18   ` [PATCH v2 2/2] ftrace: do CPU checking after preemption disabled 王贇
  2021-10-13  3:26   ` [PATCH v2 0/2] fix & prevent the missing preemption disabling Steven Rostedt
  2 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  3:17 UTC (permalink / raw)
  To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

As the documentation explained, ftrace_test_recursion_trylock()
and ftrace_test_recursion_unlock() were supposed to disable and
enable preemption properly, however currently this work is done
outside of the function, which could be missing by mistake.

This path will make sure the preemption was disabled when trylock()
succeed, and the unlock() will enable the preemption if previously
enabled.

CC: Steven Rostedt <rostedt@goodmis.org>
CC: Miroslav Benes <mbenes@suse.cz>
Reported-by: Abaci <abaci@linux.alibaba.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
 arch/csky/kernel/probes/ftrace.c     |  2 --
 arch/parisc/kernel/ftrace.c          |  2 --
 arch/powerpc/kernel/kprobes-ftrace.c |  2 --
 arch/riscv/kernel/probes/ftrace.c    |  2 --
 arch/x86/kernel/kprobes/ftrace.c     |  2 --
 include/linux/trace_recursion.h      | 22 +++++++++++++++++++++-
 kernel/livepatch/patch.c             |  6 ------
 kernel/trace/trace_functions.c       |  5 -----
 8 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c
index b388228..834cffc 100644
--- a/arch/csky/kernel/probes/ftrace.c
+++ b/arch/csky/kernel/probes/ftrace.c
@@ -17,7 +17,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		return;

 	regs = ftrace_get_regs(fregs);
-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (!p) {
 		p = get_kprobe((kprobe_opcode_t *)(ip - MCOUNT_INSN_SIZE));
@@ -57,7 +56,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
index 0a1e75a..3543496 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -216,7 +216,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		return;

 	regs = ftrace_get_regs(fregs);
-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))
 		goto out;
@@ -245,7 +244,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	}
 	__this_cpu_write(current_kprobe, NULL);
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c
index 7154d58..072ebe7 100644
--- a/arch/powerpc/kernel/kprobes-ftrace.c
+++ b/arch/powerpc/kernel/kprobes-ftrace.c
@@ -26,7 +26,6 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
 		return;

 	regs = ftrace_get_regs(fregs);
-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)nip);
 	if (unlikely(!p) || kprobe_disabled(p))
 		goto out;
@@ -61,7 +60,6 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/riscv/kernel/probes/ftrace.c b/arch/riscv/kernel/probes/ftrace.c
index aab85a8..7142ec4 100644
--- a/arch/riscv/kernel/probes/ftrace.c
+++ b/arch/riscv/kernel/probes/ftrace.c
@@ -15,7 +15,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (bit < 0)
 		return;

-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))
 		goto out;
@@ -52,7 +51,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 596de2f..dd2ec14 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -25,7 +25,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (bit < 0)
 		return;

-	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))
 		goto out;
@@ -59,7 +58,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 out:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
index a9f9c57..101e1fb 100644
--- a/include/linux/trace_recursion.h
+++ b/include/linux/trace_recursion.h
@@ -208,13 +208,29 @@ static __always_inline void trace_clear_recursion(int bit)
  * Use this for ftrace callbacks. This will detect if the function
  * tracing recursed in the same context (normal vs interrupt),
  *
+ * The ftrace_test_recursion_trylock() will disable preemption,
+ * which is required for the variant of synchronize_rcu() that is
+ * used to allow patching functions where RCU is not watching.
+ * See klp_synchronize_transition() for more details.
+ *
  * Returns: -1 if a recursion happened.
  *           >= 0 if no recursion
  */
 static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
 							 unsigned long parent_ip)
 {
-	return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
+	int bit;
+
+	bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
+	/*
+	 * The zero bit indicate we are nested
+	 * in another trylock(), which means the
+	 * preemption already disabled.
+	 */
+	if (bit > 0)
+		preempt_disable_notrace();
+
+	return bit;
 }

 /**
@@ -222,9 +238,13 @@ static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
  * @bit: The return of a successful ftrace_test_recursion_trylock()
  *
  * This is used at the end of a ftrace callback.
+ *
+ * Preemption will be enabled (if it was previously enabled).
  */
 static __always_inline void ftrace_test_recursion_unlock(int bit)
 {
+	if (bit)
+		preempt_enable_notrace();
 	trace_clear_recursion(bit);
 }

diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index e8029ae..6e66ccd 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	bit = ftrace_test_recursion_trylock(ip, parent_ip);
 	if (WARN_ON_ONCE(bit < 0))
 		return;
-	/*
-	 * A variant of synchronize_rcu() is used to allow patching functions
-	 * where RCU is not watching, see klp_synchronize_transition().
-	 */
-	preempt_disable_notrace();

 	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
 				      stack_node);
@@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	klp_arch_set_pc(fregs, (unsigned long)func->new_func);

 unlock:
-	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
 }

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 1f0e63f..9f1bfbe 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -186,7 +186,6 @@ static void function_trace_start(struct trace_array *tr)
 		return;

 	trace_ctx = tracing_gen_ctx();
-	preempt_disable_notrace();

 	cpu = smp_processor_id();
 	data = per_cpu_ptr(tr->array_buffer.data, cpu);
@@ -194,7 +193,6 @@ static void function_trace_start(struct trace_array *tr)
 		trace_function(tr, ip, parent_ip, trace_ctx);

 	ftrace_test_recursion_unlock(bit);
-	preempt_enable_notrace();
 }

 #ifdef CONFIG_UNWINDER_ORC
@@ -298,8 +296,6 @@ static inline void process_repeats(struct trace_array *tr,
 	if (bit < 0)
 		return;

-	preempt_disable_notrace();
-
 	cpu = smp_processor_id();
 	data = per_cpu_ptr(tr->array_buffer.data, cpu);
 	if (atomic_read(&data->disabled))
@@ -324,7 +320,6 @@ static inline void process_repeats(struct trace_array *tr,

 out:
 	ftrace_test_recursion_unlock(bit);
-	preempt_enable_notrace();
 }

 static void
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/2] ftrace: do CPU checking after preemption disabled
  2021-10-13  3:16 ` [PATCH v2 0/2] fix & prevent the missing preemption disabling 王贇
  2021-10-13  3:17   ` [PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock() 王贇
@ 2021-10-13  3:18   ` 王贇
  2021-10-13  3:26   ` [PATCH v2 0/2] fix & prevent the missing preemption disabling Steven Rostedt
  2 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  3:18 UTC (permalink / raw)
  To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
	Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
	Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
	Joe Lawrence, Colin Ian King, Masami Hiramatsu,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

With CONFIG_DEBUG_PREEMPT we observed reports like:

  BUG: using smp_processor_id() in preemptible
  caller is perf_ftrace_function_call+0x6f/0x2e0
  CPU: 1 PID: 680 Comm: a.out Not tainted
  Call Trace:
   <TASK>
   dump_stack_lvl+0x8d/0xcf
   check_preemption_disabled+0x104/0x110
   ? optimize_nops.isra.7+0x230/0x230
   ? text_poke_bp_batch+0x9f/0x310
   perf_ftrace_function_call+0x6f/0x2e0
   ...
   __text_poke+0x5/0x620
   text_poke_bp_batch+0x9f/0x310

This telling us the CPU could be changed after task is preempted, and
the checking on CPU before preemption will be invalid.

Since now ftrace_test_recursion_trylock() will help to disable the
preemption, this patch just do the checking after trylock() to address
the issue.

CC: Steven Rostedt <rostedt@goodmis.org>
Reported-by: Abaci <abaci@linux.alibaba.com>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
 kernel/trace/trace_event_perf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 6aed10e..fba8cb7 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -441,13 +441,13 @@ void perf_trace_buf_update(void *record, u16 type)
 	if (!rcu_is_watching())
 		return;

-	if ((unsigned long)ops->private != smp_processor_id())
-		return;
-
 	bit = ftrace_test_recursion_trylock(ip, parent_ip);
 	if (bit < 0)
 		return;

+	if ((unsigned long)ops->private != smp_processor_id())
+		goto out;
+
 	event = container_of(ops, struct perf_event, ftrace_ops);

 	/*
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 0/2] fix & prevent the missing preemption disabling
  2021-10-13  3:16 ` [PATCH v2 0/2] fix & prevent the missing preemption disabling 王贇
  2021-10-13  3:17   ` [PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock() 王贇
  2021-10-13  3:18   ` [PATCH v2 2/2] ftrace: do CPU checking after preemption disabled 王贇
@ 2021-10-13  3:26   ` Steven Rostedt
  2021-10-13  3:33     ` 王贇
  2 siblings, 1 reply; 24+ messages in thread
From: Steven Rostedt @ 2021-10-13  3:26 UTC (permalink / raw)
  To: 王贇
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching

Please start a new thread when sending new versions. v2 should not be a
reply to v1. If you want to reference v1, just add it to the cover
letter with a link tag:

Link: https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda395e@linux.alibaba.com/

-- Steve


On Wed, 13 Oct 2021 11:16:56 +0800
王贇 <yun.wang@linux.alibaba.com> wrote:

> The testing show that perf_ftrace_function_call() are using smp_processor_id()
> with preemption enabled, all the checking on CPU could be wrong after preemption.
> 
> As Peter point out, the section between ftrace_test_recursion_trylock/unlock()
> pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst'
> explained, but currently the work is done outside of the helpers.
> 
> Patch 1/2 will make sure preemption disabled after trylock() succeed,
> patch 2/2 will do smp_processor_id() checking after trylock to address the
> issue.
> 
> Michael Wang (2):
>   ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
>   ftrace: do CPU checking after preemption disabled
> 
>  arch/csky/kernel/probes/ftrace.c     |  2 --
>  arch/parisc/kernel/ftrace.c          |  2 --
>  arch/powerpc/kernel/kprobes-ftrace.c |  2 --
>  arch/riscv/kernel/probes/ftrace.c    |  2 --
>  arch/x86/kernel/kprobes/ftrace.c     |  2 --
>  include/linux/trace_recursion.h      | 22 +++++++++++++++++++++-
>  kernel/livepatch/patch.c             |  6 ------
>  kernel/trace/trace_event_perf.c      |  6 +++---
>  kernel/trace/trace_functions.c       |  5 -----
>  9 files changed, 24 insertions(+), 25 deletions(-)
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 0/2] fix & prevent the missing preemption disabling
  2021-10-13  3:26   ` [PATCH v2 0/2] fix & prevent the missing preemption disabling Steven Rostedt
@ 2021-10-13  3:33     ` 王贇
  0 siblings, 0 replies; 24+ messages in thread
From: 王贇 @ 2021-10-13  3:33 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Guo Ren, Ingo Molnar, James E.J. Bottomley, Helge Deller,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Thomas Gleixner,
	Borislav Petkov, x86, H. Peter Anvin, Josh Poimboeuf,
	Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence,
	Colin Ian King, Masami Hiramatsu, Peter Zijlstra (Intel),
	Nicholas Piggin, Jisheng Zhang, linux-csky, linux-kernel,
	linux-parisc, linuxppc-dev, linux-riscv, live-patching



On 2021/10/13 上午11:26, Steven Rostedt wrote:
> Please start a new thread when sending new versions. v2 should not be a
> reply to v1. If you want to reference v1, just add it to the cover
> letter with a link tag:
> 
> Link: https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda395e@linux.alibaba.com/

Ok, I'll resend it with link then.

Regards,
Michael Wang


> 
> -- Steve
> 
> 
> On Wed, 13 Oct 2021 11:16:56 +0800
> 王贇 <yun.wang@linux.alibaba.com> wrote:
> 
>> The testing show that perf_ftrace_function_call() are using smp_processor_id()
>> with preemption enabled, all the checking on CPU could be wrong after preemption.
>>
>> As Peter point out, the section between ftrace_test_recursion_trylock/unlock()
>> pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst'
>> explained, but currently the work is done outside of the helpers.
>>
>> Patch 1/2 will make sure preemption disabled after trylock() succeed,
>> patch 2/2 will do smp_processor_id() checking after trylock to address the
>> issue.
>>
>> Michael Wang (2):
>>   ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
>>   ftrace: do CPU checking after preemption disabled
>>
>>  arch/csky/kernel/probes/ftrace.c     |  2 --
>>  arch/parisc/kernel/ftrace.c          |  2 --
>>  arch/powerpc/kernel/kprobes-ftrace.c |  2 --
>>  arch/riscv/kernel/probes/ftrace.c    |  2 --
>>  arch/x86/kernel/kprobes/ftrace.c     |  2 --
>>  include/linux/trace_recursion.h      | 22 +++++++++++++++++++++-
>>  kernel/livepatch/patch.c             |  6 ------
>>  kernel/trace/trace_event_perf.c      |  6 +++---
>>  kernel/trace/trace_functions.c       |  5 -----
>>  9 files changed, 24 insertions(+), 25 deletions(-)
>>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing
  2021-10-12  5:39 [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
                   ` (3 preceding siblings ...)
  2021-10-13  3:16 ` [PATCH v2 0/2] fix & prevent the missing preemption disabling 王贇
@ 2021-10-26 23:48 ` Palmer Dabbelt
  4 siblings, 0 replies; 24+ messages in thread
From: Palmer Dabbelt @ 2021-10-26 23:48 UTC (permalink / raw)
  To: yun.wang
  Cc: guoren, rostedt, mingo, James.Bottomley, deller, mpe, benh,
	paulus, Paul Walmsley, aou, tglx, bp, x86, hpa, jpoimboe, jikos,
	mbenes, pmladek, joe.lawrence, colin.king, mhiramat, peterz,
	npiggin, jszhang, linux-csky, linux-kernel, linux-parisc,
	linuxppc-dev, linux-riscv, live-patching

On Mon, 11 Oct 2021 22:39:16 PDT (-0700), yun.wang@linux.alibaba.com wrote:
> The testing show that perf_ftrace_function_call() are using
> smp_processor_id() with preemption enabled, all the checking
> on CPU could be wrong after preemption, PATCH 1/2 will fix
> that.
>
> Besides, as Peter point out, the testing of recursion within
> the section between ftrace_test_recursion_trylock()/_unlock()
> pair also need the preemption disabled as the documentation
> explained, PATCH 2/2 will make sure on that.
>
> Michael Wang (2):
>   ftrace: disable preemption on the testing of recursion
>   ftrace: prevent preemption in perf_ftrace_function_call()
>
>  arch/csky/kernel/probes/ftrace.c     |  2 --
>  arch/parisc/kernel/ftrace.c          |  2 --
>  arch/powerpc/kernel/kprobes-ftrace.c |  2 --
>  arch/riscv/kernel/probes/ftrace.c    |  2 --
>  arch/x86/kernel/kprobes/ftrace.c     |  2 --
>  include/linux/trace_recursion.h      | 10 +++++++++-
>  kernel/livepatch/patch.c             |  6 ------
>  kernel/trace/trace_event_perf.c      | 17 +++++++++++++----
>  kernel/trace/trace_functions.c       |  5 -----
>  9 files changed, 22 insertions(+), 26 deletions(-)

Acked-by: Palmer Dabbelt <palmerdabbelt@google.com> # RISC-V

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-10-26 23:48 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12  5:39 [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
2021-10-12  5:40 ` [PATCH 1/2] ftrace: disable preemption on the testing of recursion 王贇
2021-10-12 12:17   ` Steven Rostedt
2021-10-13  1:46     ` 王贇
2021-10-12 12:24   ` Miroslav Benes
2021-10-12 12:29     ` Steven Rostedt
2021-10-13  1:52       ` 王贇
2021-10-13  1:50     ` 王贇
2021-10-13  2:27       ` Steven Rostedt
2021-10-13  2:36         ` 王贇
2021-10-12 12:43   ` Steven Rostedt
2021-10-13  2:04     ` 王贇
2021-10-13  2:30       ` Steven Rostedt
2021-10-13  2:38         ` 王贇
2021-10-12  5:40 ` [PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call() 王贇
2021-10-12 11:20   ` Peter Zijlstra
2021-10-13  1:45     ` 王贇
2021-10-12  5:41 ` [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing 王贇
2021-10-13  3:16 ` [PATCH v2 0/2] fix & prevent the missing preemption disabling 王贇
2021-10-13  3:17   ` [PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock() 王贇
2021-10-13  3:18   ` [PATCH v2 2/2] ftrace: do CPU checking after preemption disabled 王贇
2021-10-13  3:26   ` [PATCH v2 0/2] fix & prevent the missing preemption disabling Steven Rostedt
2021-10-13  3:33     ` 王贇
2021-10-26 23:48 ` [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing Palmer Dabbelt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).