from:"王贇"

[PATCH v7 2/2] ftrace: do CPU checking after preemption disabled

2021-10-26 Thread 王贇

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:
   
   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 
Reported-by: Abaci 
Signed-off-by: Michael Wang 
---
 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

[PATCH v7 1/2] ftrace: disable preemption when recursion locked

2021-10-26 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption disabled, we
can just merge the logical.

This patch will make sure the preemption has been disabled when
trace_test_and_set_recursion() return bit >= 0, and
trace_clear_recursion() will enable the preemption if previously
enabled.

CC: Petr Mladek 
CC: Steven Rostedt 
CC: Miroslav Benes 
Reported-by: Abaci 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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  | 11 ++-
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_functions.c   |  5 -
 9 files changed, 22 insertions(+), 32 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 7d14242..90c4345 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -210,7 +210,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;
@@ -239,7 +238,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);

[PATCH v7 0/2] fix & prevent the missing preemption disabling

2021-10-26 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption to be disabled, we
can just merge the logical together.

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

v1: 
https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda3...@linux.alibaba.com/
v2: 
https://lore.kernel.org/all/b1d7fe43-ce84-0ed7-32f7-ea1d12d0b...@linux.alibaba.com/
v3: 
https://lore.kernel.org/all/609b565a-ed6e-a1da-f025-166691b5d...@linux.alibaba.com/
V4: 
https://lore.kernel.org/all/32a36348-69ee-6464-390c-3a8d6e9d2...@linux.alibaba.com/
V5: 
https://lore.kernel.org/all/3ca92dc9-ea04-ddc2-71cd-524bfa5a5...@linux.alibaba.com/
V6: 
https://lore.kernel.org/all/df8e9b3e-504c-635d-4e92-99cdf9f05...@linux.alibaba.com/

Michael Wang (2):
  ftrace: disable preemption when recursion locked
  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  | 11 ++-
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_event_perf.c  |  6 +++---
 kernel/trace/trace_functions.c   |  5 -
 10 files changed, 25 insertions(+), 35 deletions(-)

-- 
1.8.3.1

Re: [PATCH v6 1/2] ftrace: disable preemption when recursion locked

2021-10-26 Thread 王贇




On 2021/10/27 上午10:55, Steven Rostedt wrote:
> On Wed, 27 Oct 2021 10:34:13 +0800
> 王贇  wrote:
> 
>> +/*
>> + * Preemption will be enabled (if it was previously enabled).
>> + */
>>  static __always_inline void trace_clear_recursion(int bit)
>>  {
>> +WARN_ON_ONCE(bit < 0);
> 
> Can you send a v7 without the WARN_ON.
> 
> This is an extremely hot path, and this will cause noticeable overhead.
> 
> If something were to call this with bit < 0, then it would crash and
> burn rather quickly.

I see, if the problem will be notified anyway then it's fine, v7 on the way.

Regards,
Michael Wang

> 
> -- Steve
> 
> 
>> +
>> +preempt_enable_notrace();
>>  barrier();
>>  trace_recursion_clear(bit);
>>  }

[PATCH v6 2/2] ftrace: do CPU checking after preemption disabled

2021-10-26 Thread 王贇

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:
   
   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 
Reported-by: Abaci 
Signed-off-by: Michael Wang 
---
 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

[PATCH v6 1/2] ftrace: disable preemption when recursion locked

2021-10-26 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption disabled, we
can just merge the logical.

This patch will make sure the preemption has been disabled when
trace_test_and_set_recursion() return bit >= 0, and
trace_clear_recursion() will enable the preemption if previously
enabled.

CC: Petr Mladek 
CC: Steven Rostedt 
CC: Miroslav Benes 
Reported-by: Abaci 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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  | 13 -
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_functions.c   |  5 -
 9 files changed, 24 insertions(+), 32 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 7d14242..90c4345 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -210,7 +210,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;
@@ -239,7 +238,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);

[PATCH v6 0/2] fix & prevent the missing preemption disabling

2021-10-26 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption to be disabled, we
can just merge the logical together.

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

v1: 
https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda3...@linux.alibaba.com/
v2: 
https://lore.kernel.org/all/b1d7fe43-ce84-0ed7-32f7-ea1d12d0b...@linux.alibaba.com/
v3: 
https://lore.kernel.org/all/609b565a-ed6e-a1da-f025-166691b5d...@linux.alibaba.com/
V4: 
https://lore.kernel.org/all/32a36348-69ee-6464-390c-3a8d6e9d2...@linux.alibaba.com/
V5: 
https://lore.kernel.org/all/3ca92dc9-ea04-ddc2-71cd-524bfa5a5...@linux.alibaba.com/

Michael Wang (2):
  ftrace: disable preemption when recursion locked
  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  | 13 -
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_event_perf.c  |  6 +++---
 kernel/trace/trace_functions.c   |  5 -
 10 files changed, 27 insertions(+), 35 deletions(-)

-- 
1.8.3.1

Re: [PATCH v5 1/2] ftrace: disable preemption when recursion locked

2021-10-26 Thread 王贇




On 2021/10/27 上午10:26, Steven Rostedt wrote:
> On Wed, 27 Oct 2021 09:54:13 +0800
> 王贇  wrote:
> 
>> My apologize for the stupid comments... I'll send a v6 for this patch
>> only to fix that, please let me know if this is not a good way to fix
>> few lines of comments.
> 
> Actually, please resend both patches, as a new patch set, on its own thread.
> 
> Just replying here won't trigger my patchwork scripts.
> 
> And also, if you don't include the other patch, the scripts will drop
> it.

Got it~

Regards,
Michael Wang

> 
> Thanks,
> 
> -- Steve
>

Re: [PATCH v6] ftrace: disable preemption when recursion locked

2021-10-26 Thread 王贇

Hi, Steven, Miroslav

Should have fixed the comments about bit value, besides, add
a warn in trace_clear_recursion() to make sure the bit < 0
abusing case will get notified.

Please let me know if there are any other issues :-)

Regards,
Michael Wang

On 2021/10/27 上午10:11, 王贇 wrote:
> 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.
> 
> And since the internal using of trace_test_and_set_recursion()
> and trace_clear_recursion() also require preemption disabled, we
> can just merge the logical.
> 
> This patch will make sure the preemption has been disabled when
> trace_test_and_set_recursion() return bit >= 0, and
> trace_clear_recursion() will enable the preemption if previously
> enabled.
> 
> CC: Petr Mladek 
> CC: Steven Rostedt 
> CC: Miroslav Benes 
> Reported-by: Abaci 
> Suggested-by: Peter Zijlstra 
> Signed-off-by: Michael Wang 
> ---
>  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  | 13 -
>  kernel/livepatch/patch.c | 13 +++--
>  kernel/trace/ftrace.c| 15 +--
>  kernel/trace/trace_functions.c   |  5 -
>  9 files changed, 24 insertions(+), 32 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 7d14242..90c4345 100644
> --- a/arch/parisc/kernel/ftrace.c
> +++ b/arch/parisc/kernel/ftrace.c
> @@ -210,7 +210,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;
> @@ -239,7 +238,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,
>

[PATCH v6] ftrace: disable preemption when recursion locked

2021-10-26 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption disabled, we
can just merge the logical.

This patch will make sure the preemption has been disabled when
trace_test_and_set_recursion() return bit >= 0, and
trace_clear_recursion() will enable the preemption if previously
enabled.

CC: Petr Mladek 
CC: Steven Rostedt 
CC: Miroslav Benes 
Reported-by: Abaci 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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  | 13 -
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_functions.c   |  5 -
 9 files changed, 24 insertions(+), 32 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 7d14242..90c4345 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -210,7 +210,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;
@@ -239,7 +238,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);

Re: [PATCH v5 1/2] ftrace: disable preemption when recursion locked

2021-10-26 Thread 王贇




On 2021/10/26 下午8:01, Steven Rostedt wrote:
> On Tue, 26 Oct 2021 17:48:10 +0800
> 王贇  wrote:
> 
>>> The two comments should be updated too since Steven removed the "bit == 0" 
>>> trick.  
>>
>> Could you please give more hint on how will it be correct?
>>
>> I get the point that bit will no longer be 0, there are only -1 or > 0 now
>> so trace_test_and_set_recursion() will disable preemption on bit > 0 and
>> trace_clear_recursion() will enabled it since it should only be called when
>> bit > 0 (I remember we could use a WARN_ON here now :-P).
>>
>>>   
>>>> @@ -178,7 +187,7 @@ static __always_inline void trace_clear_recursion(int 
>>>> bit)
>>>>   * tracing recursed in the same context (normal vs interrupt),
>>>>   *
>>>>   * Returns: -1 if a recursion happened.
>>>> - *   >= 0 if no recursion
>>>> + *   > 0 if no recursion.
>>>>   */
>>>>  static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
>>>> unsigned long 
>>>> parent_ip)  
>>>
>>> And this change would not be correct now.  
>>
>> I thought it will no longer return 0 so I change it to > 0, isn't that 
>> correct?
> 
> No it is not. I removed the bit + 1 return value, which means it returns the
> actual bit now. Which is 0 or more.

Ah, the return is bit not val, I must be drunk...

My apologize for the stupid comments... I'll send a v6 for this patch
only to fix that, please let me know if this is not a good way to fix
few lines of comments.

Regards,
Michael Wang

> 
> -- Steve
>

Re: [PATCH v5 1/2] ftrace: disable preemption when recursion locked

2021-10-26 Thread 王贇

Hi, Miroslav

On 2021/10/26 下午5:35, Miroslav Benes wrote:
> Hi,
> 
>> diff --git a/include/linux/trace_recursion.h 
>> b/include/linux/trace_recursion.h
>> index abe1a50..2bc1522 100644
>> --- a/include/linux/trace_recursion.h
>> +++ b/include/linux/trace_recursion.h
>> @@ -135,6 +135,9 @@ static __always_inline int trace_get_context_bit(void)
>>  # define do_ftrace_record_recursion(ip, pip)do { } while (0)
>>  #endif
>>
>> +/*
>> + * Preemption is promised to be disabled when return bit > 0.
>> + */
>>  static __always_inline int trace_test_and_set_recursion(unsigned long ip, 
>> unsigned long pip,
>>  int start)
>>  {
>> @@ -162,11 +165,17 @@ static __always_inline int 
>> trace_test_and_set_recursion(unsigned long ip, unsign
>>  current->trace_recursion = val;
>>  barrier();
>>
>> +preempt_disable_notrace();
>> +
>>  return bit;
>>  }
>>
>> +/*
>> + * Preemption will be enabled (if it was previously enabled).
>> + */
>>  static __always_inline void trace_clear_recursion(int bit)
>>  {
>> +preempt_enable_notrace();
>>  barrier();
>>  trace_recursion_clear(bit);
>>  }
> 
> The two comments should be updated too since Steven removed the "bit == 0" 
> trick.

Could you please give more hint on how will it be correct?

I get the point that bit will no longer be 0, there are only -1 or > 0 now
so trace_test_and_set_recursion() will disable preemption on bit > 0 and
trace_clear_recursion() will enabled it since it should only be called when
bit > 0 (I remember we could use a WARN_ON here now :-P).

> 
>> @@ -178,7 +187,7 @@ static __always_inline void trace_clear_recursion(int 
>> bit)
>>   * tracing recursed in the same context (normal vs interrupt),
>>   *
>>   * Returns: -1 if a recursion happened.
>> - *   >= 0 if no recursion
>> + *   > 0 if no recursion.
>>   */
>>  static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
>>   unsigned long 
>> parent_ip)
> 
> And this change would not be correct now.

I thought it will no longer return 0 so I change it to > 0, isn't that correct?

Regards,
Michael Wang

> 
> Regards
> Miroslav
>

[PATCH v5 2/2] ftrace: do CPU checking after preemption disabled

2021-10-25 Thread 王贇

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:
   
   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 
Reported-by: Abaci 
Signed-off-by: Michael Wang 
---
 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

[PATCH v5 1/2] ftrace: disable preemption when recursion locked

2021-10-25 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption disabled, we
can just merge the logical.

This patch will make sure the preemption has been disabled when
trace_test_and_set_recursion() return bit >= 0, and
trace_clear_recursion() will enable the preemption if previously
enabled.

CC: Petr Mladek 
CC: Steven Rostedt 
CC: Miroslav Benes 
Reported-by: Abaci 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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  | 11 ++-
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_functions.c   |  5 -
 9 files changed, 22 insertions(+), 32 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 7d14242..90c4345 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -210,7 +210,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;
@@ -239,7 +238,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);

[PATCH v5 0/2] fix & prevent the missing preemption disabling

2021-10-25 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption to be disabled, we
can just merge the logical together.

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

v1: 
https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda3...@linux.alibaba.com/
v2: 
https://lore.kernel.org/all/b1d7fe43-ce84-0ed7-32f7-ea1d12d0b...@linux.alibaba.com/
v3: 
https://lore.kernel.org/all/609b565a-ed6e-a1da-f025-166691b5d...@linux.alibaba.com/
V4: 
https://lore.kernel.org/all/32a36348-69ee-6464-390c-3a8d6e9d2...@linux.alibaba.com/

Michael Wang (2):
  ftrace: disable preemption when recursion locked
  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  | 11 ++-
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_event_perf.c  |  6 +++---
 kernel/trace/trace_functions.c   |  5 -
 10 files changed, 25 insertions(+), 35 deletions(-)

-- 
1.8.3.1

Re: [PATCH v4 0/2] fix & prevent the missing preemption disabling

2021-10-25 Thread 王贇




On 2021/10/26 上午10:42, Steven Rostedt wrote:
> On Tue, 26 Oct 2021 10:09:12 +0800
> 王贇  wrote:
> 
>> Just a ping, to see if there are any more comments :-P
> 
> I guess you missed what went into mainline (and your name found a bug
> in my perl script for importing patches ;-)
> 
>   https://lore.kernel.org/all/20211019091344.65629...@gandalf.local.home/

Cool~ Missing some chinese font maybe, that's fine :-)

> 
> Which means patch 1 needs to change:
>> +/*
>> + * Disable preemption to fulfill the promise.
>> + *
>> + * Don't worry about the bit 0 cases, they indicate
>> + * the disabling behaviour has already been done by
>> + * internal call previously.
>> + */
>> +preempt_disable_notrace();
>> +
>>  return bit + 1;
>>  }
>>
>> +/*
>> + * Preemption will be enabled (if it was previously enabled).
>> + */
>>  static __always_inline void trace_clear_recursion(int bit)
>>  {
>>  if (!bit)
>>  return;
>>
>> +if (bit > 0)
>> +preempt_enable_notrace();
>> +
> 
> Where this wont work anymore.
> 
> Need to preempt disable and enable always.

Yup, will rebase on the latest changes~

Regards,
Michael Wang

> 
> -- Steve
> 
> 
>>  barrier();
>>  bit--;
>>  trace_recursion_clear(bit);
>> @@ -209,7 +227,7 @@ static __always_inline void trace_clear_recursion(int 
>> bit)
>>   * tracing recursed in the same context (normal vs interrupt),
>>   *

Re: [PATCH v4 0/2] fix & prevent the missing preemption disabling

2021-10-25 Thread 王贇

Just a ping, to see if there are any more comments :-P

Regards,
Michael Wang

On 2021/10/18 上午11:38, 王贇 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.
> 
> And since the internal using of trace_test_and_set_recursion()
> and trace_clear_recursion() also require preemption to be disabled, we
> can just merge the logical together.
> 
> Patch 1/2 will make sure preemption disabled when recursion lock succeed,
> patch 2/2 will do smp_processor_id() checking after trylock() to address the
> issue.
> 
> v1: 
> https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda3...@linux.alibaba.com/
> v2: 
> https://lore.kernel.org/all/b1d7fe43-ce84-0ed7-32f7-ea1d12d0b...@linux.alibaba.com/
> v3: 
> https://lore.kernel.org/all/609b565a-ed6e-a1da-f025-166691b5d...@linux.alibaba.com/
> 
> Michael Wang (2):
>   ftrace: disable preemption when recursion locked
>   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  | 20 +++-
>  kernel/livepatch/patch.c | 13 +++--
>  kernel/trace/ftrace.c| 15 +--
>  kernel/trace/trace_event_perf.c  |  6 +++---
>  kernel/trace/trace_functions.c   |  5 -
>  10 files changed, 34 insertions(+), 35 deletions(-)
>

[PATCH v4 2/2] ftrace: do CPU checking after preemption disabled

2021-10-17 Thread 王贇

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:
   
   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 
Reported-by: Abaci 
Signed-off-by: Michael Wang 
---
 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

[PATCH v4 1/2] ftrace: disable preemption when recursion locked

2021-10-17 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption disabled, we
can just merge the logical.

This patch will make sure the preemption has been disabled when
trace_test_and_set_recursion() return bit >= 0, and
trace_clear_recursion() will enable the preemption if previously
enabled.

CC: Petr Mladek 
CC: Steven Rostedt 
CC: Miroslav Benes 
Reported-by: Abaci 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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  | 20 +++-
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_functions.c   |  5 -
 9 files changed, 31 insertions(+), 32 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,

[PATCH v4 0/2] fix & prevent the missing preemption disabling

2021-10-17 Thread 王贇

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.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption to be disabled, we
can just merge the logical together.

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

v1: 
https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda3...@linux.alibaba.com/
v2: 
https://lore.kernel.org/all/b1d7fe43-ce84-0ed7-32f7-ea1d12d0b...@linux.alibaba.com/
v3: 
https://lore.kernel.org/all/609b565a-ed6e-a1da-f025-166691b5d...@linux.alibaba.com/

Michael Wang (2):
  ftrace: disable preemption when recursion locked
  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  | 20 +++-
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/ftrace.c| 15 +--
 kernel/trace/trace_event_perf.c  |  6 +++---
 kernel/trace/trace_functions.c   |  5 -
 10 files changed, 34 insertions(+), 35 deletions(-)

-- 
1.8.3.1

Re: [PATCH v3 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-15 Thread 王贇




On 2021/10/15 下午3:28, Petr Mladek wrote:
> On Fri 2021-10-15 11:13:08, 王贇 wrote:
>>
>>
>> On 2021/10/14 下午11:14, Petr Mladek wrote:
>> [snip]
>>>> -  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();
>>>
>>> Is this safe? The preemption is disabled only when
>>> trace_test_and_set_recursion() was called by 
>>> ftrace_test_recursion_trylock().
>>>
>>> We must either always disable the preemtion when bit >= 0.
>>> Or we have to disable the preemtion already in
>>> trace_test_and_set_recursion().
>>
>> Internal calling of trace_test_and_set_recursion() will disable preemption
>> on succeed, it should be safe.
> 
> trace_test_and_set_recursion() does _not_ disable preemtion!
> It works only because all callers disable the preemption.

Yup.

> 
> It means that the comment is wrong. It is not guarantted that the
> preemption will be disabled. It works only by chance.
> 
> 
>> We can also consider move the logical into trace_test_and_set_recursion()
>> and trace_clear_recursion(), but I'm not very sure about that... ftrace
>> internally already make sure preemption disabled
> 
> How? Because it calls trace_test_and_set_recursion() via the trylock() API?

I mean currently all the direct caller of trace_test_and_set_recursion()
have disabled the preemption as you mentioned above, but yes if anyone later
write some kernel code to call trace_test_and_set_recursion() without
disabling preemption after, then promise broken.

> 
> 
>> , what uncovered is those users who call API trylock/unlock, isn't
>> it?
> 
> And this is exactly the problem. trace_test_and_set_recursion() is in
> a public header. Anyone could use it. And if anyone uses it in the
> future without the trylock() and does not disable the preemtion
> explicitely then we are lost again.
> 
> And it is even more dangerous. The original code disabled the
> preemtion on various layers. As a result, the preemtion was disabled
> several times for sure. It means that the deeper layers were
> always on the safe side.
> 
> With this patch, if the first trace_test_and_set_recursion() caller
> does not disable preemtion then trylock() will not disable it either
> and the entire code is procceed with preemtion enabled.

Yes, what confusing me at first is that I think people who call
trace_test_and_set_recursion() without trylock() can only be a
ftrace hacker, not a user, but in case if anyone can use it without
respect to preemption stuff, then I think the logical should be inside
trace_test_and_set_recursion() rather than trylock().

> 
> 
>>> Finally, the comment confused me a lot. The difference between nesting and
>>> recursion is far from clear. And the code is tricky liky like hell :-)
>>> I propose to add some comments, see below for a proposal.
>> The comments do confusing, I'll make it something like:
>>
>> The zero bit indicate trace recursion happened, whatever
>> the recursively call was made by ftrace handler or ftrace
>> itself, the preemption already disabled.
> 
> I am sorry but it is still confusing. We need to find a better way
> how to clearly explain the difference between the safe and
> unsafe recursion.
> 
> My understanding is that the recursion is:
> 
>   + "unsafe" when the trace code recursively enters the same trace point.
> 
>   + "safe" when ftrace_test_recursion_trylock() is called recursivelly
> while still processing the same trace entry.

Maybe take some example would be easier to understand...

Currently there are two way of using ftrace_test_recursion_trylock(),
one with TRACE_FTRACE_XXX we mark as A, one with TRACE_LIST_XXX we mark
as B, then:

A followed by B on same context got bit > 0
B followed by A on any context got bit 0
A followed by A on same context got bit > 0
A followed by A followed by A on same context got bit -1
B followed by B on same context got bit > 0
B followed by B followed by B on same context got bit -1

If we get rid of the TRACE_TRANSITION_BIT which allowed recursion for
onetime, then it would be:

A followed by B on same context got bit > 0
B followed by A on any context got bit 0
A followed

Re: [PATCH v3 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-14 Thread 王贇




On 2021/10/15 上午11:13, 王贇 wrote:
[snip]
>>  # define do_ftrace_record_recursion(ip, pip)do { } while (0)
>>  #endif
>>  
>> +/*
>> + * trace_test_and_set_recursion() is called on several layers
>> + * of the ftrace code when handling the same ftrace entry.
>> + * These calls might be nested/recursive.
>> + *
>> + * It uses TRACE_LIST_*BITs to distinguish between this
>> + * internal recursion and recursion caused by calling
>> + * the traced function by the ftrace code.
>> + *
>> + * Returns: > 0 when no recursion
>> + *  0 when called recursively internally (safe)
> 
> The 0 can also happened when ftrace handler recursively called trylock()
> under the same context, or not?
> 

Never mind... you're right about this.

Regards,
Michael Wang

> Regards,
> Michael Wang
> 
>> + *  -1 when the traced function was called recursively from
>> + * the ftrace handler (unsafe)
>> + */
>>  static __always_inline int trace_test_and_set_recursion(unsigned long ip, 
>> unsigned long pip,
>>  int start, int max)
>>  {
>>  unsigned int val = READ_ONCE(current->trace_recursion);
>>  int bit;
>>  
>> -/* A previous recursion check was made */
>> +/* Called recursively internally by different ftrace code layers? */
>>  if ((val & TRACE_CONTEXT_MASK) > max)
>>  return 0;
> 
>>  
>>

Re: [PATCH v3 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-14 Thread 王贇




On 2021/10/14 下午11:14, Petr Mladek wrote:
[snip]
>> -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();
> 
> Is this safe? The preemption is disabled only when
> trace_test_and_set_recursion() was called by ftrace_test_recursion_trylock().
> 
> We must either always disable the preemtion when bit >= 0.
> Or we have to disable the preemtion already in
> trace_test_and_set_recursion().

Internal calling of trace_test_and_set_recursion() will disable preemption
on succeed, it should be safe.

We can also consider move the logical into trace_test_and_set_recursion()
and trace_clear_recursion(), but I'm not very sure about that... ftrace
internally already make sure preemption disabled, what uncovered is those
users who call API trylock/unlock, isn't it?

> 
> 
> Finally, the comment confused me a lot. The difference between nesting and
> recursion is far from clear. And the code is tricky liky like hell :-)
> I propose to add some comments, see below for a proposal.
The comments do confusing, I'll make it something like:

The zero bit indicate trace recursion happened, whatever
the recursively call was made by ftrace handler or ftrace
itself, the preemption already disabled.

Will this one looks better to you?

> 
>> +
>> +return bit;
>>  }
>>  /**
>> @@ -222,9 +233,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)
> 
> This is not symetric with trylock(). It should be:
> 
>   if (bit > 0)
> 
> Anyway, trace_clear_recursion() quiently ignores bit != 0

Yes, bit == 0 should not happen in here.

> 
> 
>> +preempt_enable_notrace();
>>  trace_clear_recursion(bit);
>>  }
> 
> 
> Below is my proposed patch that tries to better explain the recursion
> check:
> 
> From 20d69f11e2683262fa0043b803999061cbac543f Mon Sep 17 00:00:00 2001
> From: Petr Mladek 
> Date: Thu, 14 Oct 2021 16:57:39 +0200
> Subject: [PATCH] trace: Better describe the recursion check return values
> 
> The trace recursion check might be called recursively by different
> layers of the tracing code. It is safe recursion and the check
> is even optimized for this case.
> 
> The problematic recursion is when the traced function is called
> by the tracing code. This is properly detected.
> 
> Try to explain this difference a better way.
> 
> Signed-off-by: Petr Mladek 
> ---
>  include/linux/trace_recursion.h | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
> index a9f9c5714e65..b5efb804efdf 100644
> --- a/include/linux/trace_recursion.h
> +++ b/include/linux/trace_recursion.h
> @@ -159,13 +159,27 @@ extern void ftrace_record_recursion(unsigned long ip, 
> unsigned long parent_ip);
>  # define do_ftrace_record_recursion(ip, pip) do { } while (0)
>  #endif
>  
> +/*
> + * trace_test_and_set_recursion() is called on several layers
> + * of the ftrace code when handling the same ftrace entry.
> + * These calls might be nested/recursive.
> + *
> + * It uses TRACE_LIST_*BITs to distinguish between this
> + * internal recursion and recursion caused by calling
> + * the traced function by the ftrace code.
> + *
> + * Returns: > 0 when no recursion
> + *  0 when called recursively internally (safe)

The 0 can also happened when ftrace handler recursively called trylock()
under the same context, or not?

Regards,
Michael Wang

> + *   -1 when the traced function was called recursively from
> + * the ftrace handler (unsafe)
> + */
>  static __always_inline int trace_test_and_set_recursion(unsigned long ip, 
> unsigned long pip,
>   int start, int max)
>  {
>   unsigned int val = READ_ONCE(current->trace_recursion);
>   int bit;
>  
> - /* A previous recursion check was made */
> + /* Called recursively internally by different ftrace code layers? */
>   if ((val & TRACE_CONTEXT_MASK) > max)
>   return 0;

>  
>

Re: [PATCH v3 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-14 Thread 王贇




On 2021/10/14 下午5:13, Miroslav Benes wrote:
>> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
>> index e8029ae..b8d75fb 100644
>> --- a/kernel/livepatch/patch.c
>> +++ b/kernel/livepatch/patch.c
>> @@ -49,14 +49,16 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>>
>>  ops = container_of(fops, struct klp_ops, fops);
>>
>> +/*
>> + *
> 
> This empty line is not useful.
> 
>> + * 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.
>> + */
>>  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(>func_stack, struct klp_func,
>>stack_node);
>> @@ -120,7 +122,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);
>>  }
> 
> Acked-by: Miroslav Benes 
> 
> for the livepatch part of the patch.
> 
> I would also ask you not to submit new versions so often, so that the 
> other reviewers have time to actually review the patch set.
> 
> Quoting from Documentation/process/submitting-patches.rst:
> 
> "Wait for a minimum of one week before resubmitting or pinging reviewers - 
> possibly longer during busy times like merge windows."

Thanks for the Ack and suggestion, will take care from now on :-)

Regards,
Michael Wang

> 
> Thanks
> 
> Miroslav
>

[PATCH v3 2/2] ftrace: do CPU checking after preemption disabled

2021-10-13 Thread 王贇

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:
   
   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 
Reported-by: Abaci 
Signed-off-by: Michael Wang 
---
 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

[PATCH v3 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-13 Thread 王贇

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 
CC: Miroslav Benes 
Reported-by: Abaci 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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  | 17 -
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/trace_functions.c   |  5 -
 8 files changed, 23 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..58e474c 100644
---

[PATCH v3 0/2] fix & prevent the missing preemption disabling

2021-10-13 Thread 王贇

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.

v1: 
https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda3...@linux.alibaba.com/
v2: 
https://lore.kernel.org/all/b1d7fe43-ce84-0ed7-32f7-ea1d12d0b...@linux.alibaba.com/

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  | 17 -
 kernel/livepatch/patch.c | 13 +++--
 kernel/trace/trace_event_perf.c  |  6 +++---
 kernel/trace/trace_functions.c   |  5 -
 9 files changed, 26 insertions(+), 25 deletions(-)

-- 
1.8.3.1

Re: [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-13 Thread 王贇




On 2021/10/13 下午4:25, Miroslav Benes wrote:
>>> Side note... the comment will eventually conflict with peterz's 
>>> https://lore.kernel.org/all/20210929152429.125997...@infradead.org/.
>>
>> Steven, would you like to share your opinion on this patch?
>>
>> If klp_synchronize_transition() will be removed anyway, the comments
>> will be meaningless and we can just drop it :-P
> 
> The comment will still be needed in some form. We will handle it depending 
> on what gets merged first. peterz's patches are not ready yet.

Ok, then I'll move it before trylock() inside klp_ftrace_handler() anyway.

Regards,
Michael Wang

> 
> Miroslav
>

Re: [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-13 Thread 王贇




On 2021/10/13 下午3:55, Miroslav Benes wrote:
>> 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.
>> + *
> 
> I think that you misunderstood. Steven proposed to put the comment before 
> ftrace_test_recursion_trylock() call site in klp_ftrace_handler().

Oh, I see... thanks for pointing out :-)

> 
>>   * Returns: -1 if a recursion happened.
[snip]
>>  }
> 
> Side note... the comment will eventually conflict with peterz's 
> https://lore.kernel.org/all/20210929152429.125997...@infradead.org/.

Steven, would you like to share your opinion on this patch?

If klp_synchronize_transition() will be removed anyway, the comments
will be meaningless and we can just drop it :-P

Regards,
Michael Wang


> 
> Miroslav
>

[RESEND PATCH v2 2/2] ftrace: do CPU checking after preemption disabled

2021-10-12 Thread 王贇

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:
   
   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 
Reported-by: Abaci 
Signed-off-by: Michael Wang 
---
 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

[RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-12 Thread 王贇

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 
CC: Miroslav Benes 
Reported-by: Abaci 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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
---

[RESEND PATCH v2 0/2] fix & prevent the missing preemption disabling

2021-10-12 Thread 王贇

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.

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

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

Re: [PATCH v2 0/2] fix & prevent the missing preemption disabling

2021-10-12 Thread 王贇




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-5dbc2eda3...@linux.alibaba.com/

Ok, I'll resend it with link then.

Regards,
Michael Wang


> 
> -- Steve
> 
> 
> On Wed, 13 Oct 2021 11:16:56 +0800
> 王贇  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(-)
>>

[PATCH v2 2/2] ftrace: do CPU checking after preemption disabled

2021-10-12 Thread 王贇

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:
   
   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 
Reported-by: Abaci 
Signed-off-by: Michael Wang 
---
 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

[PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()

2021-10-12 Thread 王贇

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 
CC: Miroslav Benes 
Reported-by: Abaci 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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
---

[PATCH v2 0/2] fix & prevent the missing preemption disabling

2021-10-12 Thread 王贇

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

Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion

2021-10-12 Thread 王贇




On 2021/10/13 上午10:30, Steven Rostedt wrote:
> On Wed, 13 Oct 2021 10:04:52 +0800
> 王贇  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
>

Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion

2021-10-12 Thread 王贇




On 2021/10/13 上午10:27, Steven Rostedt wrote:
> On Wed, 13 Oct 2021 09:50:17 +0800
> 王贇  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?

Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion

2021-10-12 Thread 王贇




On 2021/10/12 下午8:43, Steven Rostedt wrote:
> On Tue, 12 Oct 2021 13:40:08 +0800
> 王贇  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;
>>  }

Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion

2021-10-12 Thread 王贇




On 2021/10/12 下午8:29, Steven Rostedt wrote:
> On Tue, 12 Oct 2021 14:24:43 +0200 (CEST)
> Miroslav Benes  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(>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
>

Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion

2021-10-12 Thread 王贇

On 2021/10/12 下午8:24, Miroslav Benes wrote:
[snip]
>>
>>  func = list_first_or_null_rcu(>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
>

Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion

2021-10-12 Thread 王贇




On 2021/10/12 下午8:17, Steven Rostedt wrote:
> On Tue, 12 Oct 2021 13:40:08 +0800
> 王贇  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(>func_stack, struct klp_func,
>>stack_node);

Re: [PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call()

2021-10-12 Thread 王贇




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

>

[PATCH 0/2] ftrace: make sure preemption disabled on recursion testing

2021-10-12 Thread 王贇

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

Re: [PATCH 0/2] ftrace: make sure preemption disabled on recursion testing

2021-10-12 Thread 王贇




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

[PATCH 2/2] ftrace: prevent preemption in perf_ftrace_function_call()

2021-10-12 Thread 王贇

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:
   
   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 
Reported-by: Abaci 
Signed-off-by: Michael Wang 
---
 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, );
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, , , 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

[PATCH 1/2] ftrace: disable preemption on the testing of recursion

2021-10-12 Thread 王贇

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 
Suggested-by: Peter Zijlstra 
Signed-off-by: Michael Wang 
---
 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
+++

47 matches

Mail list logo