Re: [PATCH v6] ftrace: disable preemption when recursion locked
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,
> __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
> +++
[PATCH v6] ftrace: disable preemption when recursion locked
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);
