Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
Hello: This patch was applied to riscv/linux.git (fixes) by Masami Hiramatsu (Google) : On Wed, 1 May 2024 09:29:56 -0700 you wrote: > If an error happens in ftrace, ftrace_kill() will prevent disarming > kprobes. Eventually, the ftrace_ops associated with the kprobes will be > freed, yet the kprobes will still be active, and when triggered, they > will use the freed memory, likely resulting in a page fault and panic. > > This behavior can be reproduced quite easily, by creating a kprobe and > then triggering a ftrace_kill(). For simplicity, we can simulate an > ftrace error with a kernel module like [1]: > > [...] Here is the summary with links: - [v3] kprobe/ftrace: bail out if ftrace was killed https://git.kernel.org/riscv/c/1a7d0890dd4a You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
On Wed, 15 May 2024 15:18:08 -0700 Stephen Brennan wrote: > Masami Hiramatsu (Google) writes: > > On Thu, 2 May 2024 01:35:16 +0800 > > Guo Ren wrote: > > > >> On Thu, May 2, 2024 at 12:30 AM Stephen Brennan > >> wrote: > >> > > >> > If an error happens in ftrace, ftrace_kill() will prevent disarming > >> > kprobes. Eventually, the ftrace_ops associated with the kprobes will be > >> > freed, yet the kprobes will still be active, and when triggered, they > >> > will use the freed memory, likely resulting in a page fault and panic. > >> > > >> > This behavior can be reproduced quite easily, by creating a kprobe and > >> > then triggering a ftrace_kill(). For simplicity, we can simulate an > >> > ftrace error with a kernel module like [1]: > >> > > >> > [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer > >> > > >> > sudo perf probe --add commit_creds > >> > sudo perf trace -e probe:commit_creds > >> > # In another terminal > >> > make > >> > sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug > >> > # Back to perf terminal > >> > # ctrl-c > >> > sudo perf probe --del commit_creds > >> > > >> > After a short period, a page fault and panic would occur as the kprobe > >> > continues to execute and uses the freed ftrace_ops. While ftrace_kill() > >> > is supposed to be used only in extreme circumstances, it is invoked in > >> > FTRACE_WARN_ON() and so there are many places where an unexpected bug > >> > could be triggered, yet the system may continue operating, possibly > >> > without the administrator noticing. If ftrace_kill() does not panic the > >> > system, then we should do everything we can to continue operating, > >> > rather than leave a ticking time bomb. > >> > > >> > Signed-off-by: Stephen Brennan > >> > --- > >> > Changes in v3: > >> > Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" > >> > variable and check it directly in the kprobe handlers. > >> > Link to v1/v2 discussion: > >> > > >> > https://lore.kernel.org/all/20240426225834.993353-1-stephen.s.bren...@oracle.com/ > >> > > >> > arch/csky/kernel/probes/ftrace.c | 3 +++ > >> > arch/loongarch/kernel/ftrace_dyn.c | 3 +++ > >> > arch/parisc/kernel/ftrace.c | 3 +++ > >> > arch/powerpc/kernel/kprobes-ftrace.c | 3 +++ > >> > arch/riscv/kernel/probes/ftrace.c| 3 +++ > >> > arch/s390/kernel/ftrace.c| 3 +++ > >> > arch/x86/kernel/kprobes/ftrace.c | 3 +++ > >> > include/linux/kprobes.h | 7 +++ > >> > kernel/kprobes.c | 6 ++ > >> > kernel/trace/ftrace.c| 1 + > >> > 10 files changed, 35 insertions(+) > >> > > >> > diff --git a/arch/csky/kernel/probes/ftrace.c > >> > b/arch/csky/kernel/probes/ftrace.c > >> > index 834cffcfbce3..7ba4b98076de 100644 > >> > --- a/arch/csky/kernel/probes/ftrace.c > >> > +++ b/arch/csky/kernel/probes/ftrace.c > >> > @@ -12,6 +12,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned > >> > long parent_ip, > >> > struct kprobe_ctlblk *kcb; > >> > struct pt_regs *regs; > >> > > >> > + if (unlikely(kprobe_ftrace_disabled)) > >> > + return; > >> > + > >> For csky part. > >> Acked-by: Guo Ren > > > > Thanks Stephen, Guo and Steve! > > > > Let me pick this to probes/for-next! > > Thank you Masami! > > I did want to check, is this the correct git tree to be watching? > > https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git/log/?h=probes/for-next > > ( I'm not trying to pressure on timing, as I know the merge window is > hectic. Just making sure I'm watching the correct place! ) Sorry, I forgot to push it from my local tree. Now it should be there. Thanks, -- Masami Hiramatsu (Google)
Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
Masami Hiramatsu (Google) writes: > On Thu, 2 May 2024 01:35:16 +0800 > Guo Ren wrote: > >> On Thu, May 2, 2024 at 12:30 AM Stephen Brennan >> wrote: >> > >> > If an error happens in ftrace, ftrace_kill() will prevent disarming >> > kprobes. Eventually, the ftrace_ops associated with the kprobes will be >> > freed, yet the kprobes will still be active, and when triggered, they >> > will use the freed memory, likely resulting in a page fault and panic. >> > >> > This behavior can be reproduced quite easily, by creating a kprobe and >> > then triggering a ftrace_kill(). For simplicity, we can simulate an >> > ftrace error with a kernel module like [1]: >> > >> > [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer >> > >> > sudo perf probe --add commit_creds >> > sudo perf trace -e probe:commit_creds >> > # In another terminal >> > make >> > sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug >> > # Back to perf terminal >> > # ctrl-c >> > sudo perf probe --del commit_creds >> > >> > After a short period, a page fault and panic would occur as the kprobe >> > continues to execute and uses the freed ftrace_ops. While ftrace_kill() >> > is supposed to be used only in extreme circumstances, it is invoked in >> > FTRACE_WARN_ON() and so there are many places where an unexpected bug >> > could be triggered, yet the system may continue operating, possibly >> > without the administrator noticing. If ftrace_kill() does not panic the >> > system, then we should do everything we can to continue operating, >> > rather than leave a ticking time bomb. >> > >> > Signed-off-by: Stephen Brennan >> > --- >> > Changes in v3: >> > Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" >> > variable and check it directly in the kprobe handlers. >> > Link to v1/v2 discussion: >> > >> > https://lore.kernel.org/all/20240426225834.993353-1-stephen.s.bren...@oracle.com/ >> > >> > arch/csky/kernel/probes/ftrace.c | 3 +++ >> > arch/loongarch/kernel/ftrace_dyn.c | 3 +++ >> > arch/parisc/kernel/ftrace.c | 3 +++ >> > arch/powerpc/kernel/kprobes-ftrace.c | 3 +++ >> > arch/riscv/kernel/probes/ftrace.c| 3 +++ >> > arch/s390/kernel/ftrace.c| 3 +++ >> > arch/x86/kernel/kprobes/ftrace.c | 3 +++ >> > include/linux/kprobes.h | 7 +++ >> > kernel/kprobes.c | 6 ++ >> > kernel/trace/ftrace.c| 1 + >> > 10 files changed, 35 insertions(+) >> > >> > diff --git a/arch/csky/kernel/probes/ftrace.c >> > b/arch/csky/kernel/probes/ftrace.c >> > index 834cffcfbce3..7ba4b98076de 100644 >> > --- a/arch/csky/kernel/probes/ftrace.c >> > +++ b/arch/csky/kernel/probes/ftrace.c >> > @@ -12,6 +12,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned >> > long parent_ip, >> > struct kprobe_ctlblk *kcb; >> > struct pt_regs *regs; >> > >> > + if (unlikely(kprobe_ftrace_disabled)) >> > + return; >> > + >> For csky part. >> Acked-by: Guo Ren > > Thanks Stephen, Guo and Steve! > > Let me pick this to probes/for-next! Thank you Masami! I did want to check, is this the correct git tree to be watching? https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git/log/?h=probes/for-next ( I'm not trying to pressure on timing, as I know the merge window is hectic. Just making sure I'm watching the correct place! ) Thanks, Stephen
Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
Christophe Leroy writes: > Le 01/05/2024 à 18:29, Stephen Brennan a écrit : >> If an error happens in ftrace, ftrace_kill() will prevent disarming >> kprobes. Eventually, the ftrace_ops associated with the kprobes will be >> freed, yet the kprobes will still be active, and when triggered, they >> will use the freed memory, likely resulting in a page fault and panic. >> >> This behavior can be reproduced quite easily, by creating a kprobe and >> then triggering a ftrace_kill(). For simplicity, we can simulate an >> ftrace error with a kernel module like [1]: >> >> [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer >> >>sudo perf probe --add commit_creds >>sudo perf trace -e probe:commit_creds >># In another terminal >>make >>sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug >># Back to perf terminal >># ctrl-c >>sudo perf probe --del commit_creds >> >> After a short period, a page fault and panic would occur as the kprobe >> continues to execute and uses the freed ftrace_ops. While ftrace_kill() >> is supposed to be used only in extreme circumstances, it is invoked in >> FTRACE_WARN_ON() and so there are many places where an unexpected bug >> could be triggered, yet the system may continue operating, possibly >> without the administrator noticing. If ftrace_kill() does not panic the >> system, then we should do everything we can to continue operating, >> rather than leave a ticking time bomb. >> >> Signed-off-by: Stephen Brennan >> --- >> Changes in v3: >>Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" >>variable and check it directly in the kprobe handlers. > > Isn't it safer to provide a fonction rather than a direct access to a > variable ? Is the concern that other code could modify this variable? If so, then I suppose the function call is safer. But the variable is not exported and I think built-in code can be trusted not to muck with it. Maybe I'm missing your point about safety though? > By the way, wouldn't it be more performant to use a static branch (jump > label) ? I agree with Steven's concern that text modification would unfortunately not be a good way to handle an error in text modification. Especially, I believe there could be deadlock risks, as static key enablement requires taking the text_mutex and the jump_label_mutex. I'd be concerned that the text_mutex could already be held in some situations where ftrace_kill() is called. But I'm not certain about that. Thanks for taking a look! Stephen
Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
On Mon, 6 May 2024 14:46:57 + Christophe Leroy wrote: > Isn't it safer to provide a fonction rather than a direct access to a > variable ? > > By the way, wouldn't it be more performant to use a static branch (jump > label) ? A static branch could work, but the point of this is that if ftrace failed, it was likely due to an issue with text modification. Do we want to stop it via text modification? -- Steve
Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
Le 01/05/2024 à 18:29, Stephen Brennan a écrit : > If an error happens in ftrace, ftrace_kill() will prevent disarming > kprobes. Eventually, the ftrace_ops associated with the kprobes will be > freed, yet the kprobes will still be active, and when triggered, they > will use the freed memory, likely resulting in a page fault and panic. > > This behavior can be reproduced quite easily, by creating a kprobe and > then triggering a ftrace_kill(). For simplicity, we can simulate an > ftrace error with a kernel module like [1]: > > [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer > >sudo perf probe --add commit_creds >sudo perf trace -e probe:commit_creds ># In another terminal >make >sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug ># Back to perf terminal ># ctrl-c >sudo perf probe --del commit_creds > > After a short period, a page fault and panic would occur as the kprobe > continues to execute and uses the freed ftrace_ops. While ftrace_kill() > is supposed to be used only in extreme circumstances, it is invoked in > FTRACE_WARN_ON() and so there are many places where an unexpected bug > could be triggered, yet the system may continue operating, possibly > without the administrator noticing. If ftrace_kill() does not panic the > system, then we should do everything we can to continue operating, > rather than leave a ticking time bomb. > > Signed-off-by: Stephen Brennan > --- > Changes in v3: >Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" >variable and check it directly in the kprobe handlers. Isn't it safer to provide a fonction rather than a direct access to a variable ? By the way, wouldn't it be more performant to use a static branch (jump label) ? Christophe
Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
On Thu, 2 May 2024 01:35:16 +0800 Guo Ren wrote: > On Thu, May 2, 2024 at 12:30 AM Stephen Brennan > wrote: > > > > If an error happens in ftrace, ftrace_kill() will prevent disarming > > kprobes. Eventually, the ftrace_ops associated with the kprobes will be > > freed, yet the kprobes will still be active, and when triggered, they > > will use the freed memory, likely resulting in a page fault and panic. > > > > This behavior can be reproduced quite easily, by creating a kprobe and > > then triggering a ftrace_kill(). For simplicity, we can simulate an > > ftrace error with a kernel module like [1]: > > > > [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer > > > > sudo perf probe --add commit_creds > > sudo perf trace -e probe:commit_creds > > # In another terminal > > make > > sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug > > # Back to perf terminal > > # ctrl-c > > sudo perf probe --del commit_creds > > > > After a short period, a page fault and panic would occur as the kprobe > > continues to execute and uses the freed ftrace_ops. While ftrace_kill() > > is supposed to be used only in extreme circumstances, it is invoked in > > FTRACE_WARN_ON() and so there are many places where an unexpected bug > > could be triggered, yet the system may continue operating, possibly > > without the administrator noticing. If ftrace_kill() does not panic the > > system, then we should do everything we can to continue operating, > > rather than leave a ticking time bomb. > > > > Signed-off-by: Stephen Brennan > > --- > > Changes in v3: > > Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" > > variable and check it directly in the kprobe handlers. > > Link to v1/v2 discussion: > > > > https://lore.kernel.org/all/20240426225834.993353-1-stephen.s.bren...@oracle.com/ > > > > arch/csky/kernel/probes/ftrace.c | 3 +++ > > arch/loongarch/kernel/ftrace_dyn.c | 3 +++ > > arch/parisc/kernel/ftrace.c | 3 +++ > > arch/powerpc/kernel/kprobes-ftrace.c | 3 +++ > > arch/riscv/kernel/probes/ftrace.c| 3 +++ > > arch/s390/kernel/ftrace.c| 3 +++ > > arch/x86/kernel/kprobes/ftrace.c | 3 +++ > > include/linux/kprobes.h | 7 +++ > > kernel/kprobes.c | 6 ++ > > kernel/trace/ftrace.c| 1 + > > 10 files changed, 35 insertions(+) > > > > diff --git a/arch/csky/kernel/probes/ftrace.c > > b/arch/csky/kernel/probes/ftrace.c > > index 834cffcfbce3..7ba4b98076de 100644 > > --- a/arch/csky/kernel/probes/ftrace.c > > +++ b/arch/csky/kernel/probes/ftrace.c > > @@ -12,6 +12,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned > > long parent_ip, > > struct kprobe_ctlblk *kcb; > > struct pt_regs *regs; > > > > + if (unlikely(kprobe_ftrace_disabled)) > > + return; > > + > For csky part. > Acked-by: Guo Ren Thanks Stephen, Guo and Steve! Let me pick this to probes/for-next! Thank you, > > > bit = ftrace_test_recursion_trylock(ip, parent_ip); > > if (bit < 0) > > return; > > diff --git a/arch/loongarch/kernel/ftrace_dyn.c > > b/arch/loongarch/kernel/ftrace_dyn.c > > index 73858c9029cc..bff058317062 100644 > > --- a/arch/loongarch/kernel/ftrace_dyn.c > > +++ b/arch/loongarch/kernel/ftrace_dyn.c > > @@ -287,6 +287,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned > > long parent_ip, > > struct kprobe *p; > > struct kprobe_ctlblk *kcb; > > > > + if (unlikely(kprobe_ftrace_disabled)) > > + return; > > + > > bit = ftrace_test_recursion_trylock(ip, parent_ip); > > if (bit < 0) > > return; > > diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c > > index 621a4b386ae4..c91f9c2e61ed 100644 > > --- a/arch/parisc/kernel/ftrace.c > > +++ b/arch/parisc/kernel/ftrace.c > > @@ -206,6 +206,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned > > long parent_ip, > > struct kprobe *p; > > int bit; > > > > + if (unlikely(kprobe_ftrace_disabled)) > > + return; > > + > > bit = ftrace_test_recursion_trylock(ip, parent_ip); > > if (bit < 0) > > return; > > diff --git a/arch/powerpc/kernel/kprobes-ftrace.c > > b/arch/powerpc/kernel/kprobes-ftrace.c > > index 072ebe7f290b..f8208c027148 100644 > > --- a/arch/powerpc/kernel/kprobes-ftrace.c > > +++ b/arch/powerpc/kernel/kprobes-ftrace.c > > @@ -21,6 +21,9 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned > > long parent_nip, > > struct pt_regs *regs; > > int bit; > > > > + if (unlikely(kprobe_ftrace_disabled)) > > + return; > > + > > bit = ftrace_test_recursion_trylock(nip, parent_nip); > > if (bit < 0) > > return; > > diff --git a/arch/riscv/kernel/probes/ftrace.c > > b/arch/riscv/kernel/probes/ftrace.c > > index
Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
On Thu, May 2, 2024 at 12:30 AM Stephen Brennan wrote: > > If an error happens in ftrace, ftrace_kill() will prevent disarming > kprobes. Eventually, the ftrace_ops associated with the kprobes will be > freed, yet the kprobes will still be active, and when triggered, they > will use the freed memory, likely resulting in a page fault and panic. > > This behavior can be reproduced quite easily, by creating a kprobe and > then triggering a ftrace_kill(). For simplicity, we can simulate an > ftrace error with a kernel module like [1]: > > [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer > > sudo perf probe --add commit_creds > sudo perf trace -e probe:commit_creds > # In another terminal > make > sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug > # Back to perf terminal > # ctrl-c > sudo perf probe --del commit_creds > > After a short period, a page fault and panic would occur as the kprobe > continues to execute and uses the freed ftrace_ops. While ftrace_kill() > is supposed to be used only in extreme circumstances, it is invoked in > FTRACE_WARN_ON() and so there are many places where an unexpected bug > could be triggered, yet the system may continue operating, possibly > without the administrator noticing. If ftrace_kill() does not panic the > system, then we should do everything we can to continue operating, > rather than leave a ticking time bomb. > > Signed-off-by: Stephen Brennan > --- > Changes in v3: > Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" > variable and check it directly in the kprobe handlers. > Link to v1/v2 discussion: > > https://lore.kernel.org/all/20240426225834.993353-1-stephen.s.bren...@oracle.com/ > > arch/csky/kernel/probes/ftrace.c | 3 +++ > arch/loongarch/kernel/ftrace_dyn.c | 3 +++ > arch/parisc/kernel/ftrace.c | 3 +++ > arch/powerpc/kernel/kprobes-ftrace.c | 3 +++ > arch/riscv/kernel/probes/ftrace.c| 3 +++ > arch/s390/kernel/ftrace.c| 3 +++ > arch/x86/kernel/kprobes/ftrace.c | 3 +++ > include/linux/kprobes.h | 7 +++ > kernel/kprobes.c | 6 ++ > kernel/trace/ftrace.c| 1 + > 10 files changed, 35 insertions(+) > > diff --git a/arch/csky/kernel/probes/ftrace.c > b/arch/csky/kernel/probes/ftrace.c > index 834cffcfbce3..7ba4b98076de 100644 > --- a/arch/csky/kernel/probes/ftrace.c > +++ b/arch/csky/kernel/probes/ftrace.c > @@ -12,6 +12,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long > parent_ip, > struct kprobe_ctlblk *kcb; > struct pt_regs *regs; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + For csky part. Acked-by: Guo Ren > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/arch/loongarch/kernel/ftrace_dyn.c > b/arch/loongarch/kernel/ftrace_dyn.c > index 73858c9029cc..bff058317062 100644 > --- a/arch/loongarch/kernel/ftrace_dyn.c > +++ b/arch/loongarch/kernel/ftrace_dyn.c > @@ -287,6 +287,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned > long parent_ip, > struct kprobe *p; > struct kprobe_ctlblk *kcb; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c > index 621a4b386ae4..c91f9c2e61ed 100644 > --- a/arch/parisc/kernel/ftrace.c > +++ b/arch/parisc/kernel/ftrace.c > @@ -206,6 +206,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned > long parent_ip, > struct kprobe *p; > int bit; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/arch/powerpc/kernel/kprobes-ftrace.c > b/arch/powerpc/kernel/kprobes-ftrace.c > index 072ebe7f290b..f8208c027148 100644 > --- a/arch/powerpc/kernel/kprobes-ftrace.c > +++ b/arch/powerpc/kernel/kprobes-ftrace.c > @@ -21,6 +21,9 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long > parent_nip, > struct pt_regs *regs; > int bit; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(nip, parent_nip); > if (bit < 0) > return; > diff --git a/arch/riscv/kernel/probes/ftrace.c > b/arch/riscv/kernel/probes/ftrace.c > index 7142ec42e889..a69dfa610aa8 100644 > --- a/arch/riscv/kernel/probes/ftrace.c > +++ b/arch/riscv/kernel/probes/ftrace.c > @@ -11,6 +11,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long > parent_ip, > struct kprobe_ctlblk *kcb; > int bit; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit =
Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
On Wed, 1 May 2024 09:29:56 -0700 Stephen Brennan wrote: > If an error happens in ftrace, ftrace_kill() will prevent disarming > kprobes. Eventually, the ftrace_ops associated with the kprobes will be > freed, yet the kprobes will still be active, and when triggered, they > will use the freed memory, likely resulting in a page fault and panic. > > This behavior can be reproduced quite easily, by creating a kprobe and > then triggering a ftrace_kill(). For simplicity, we can simulate an > ftrace error with a kernel module like [1]: > > [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer > > sudo perf probe --add commit_creds > sudo perf trace -e probe:commit_creds > # In another terminal > make > sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug > # Back to perf terminal > # ctrl-c > sudo perf probe --del commit_creds > > After a short period, a page fault and panic would occur as the kprobe > continues to execute and uses the freed ftrace_ops. While ftrace_kill() > is supposed to be used only in extreme circumstances, it is invoked in > FTRACE_WARN_ON() and so there are many places where an unexpected bug > could be triggered, yet the system may continue operating, possibly > without the administrator noticing. If ftrace_kill() does not panic the > system, then we should do everything we can to continue operating, > rather than leave a ticking time bomb. > > Signed-off-by: Stephen Brennan > --- > Changes in v3: > Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" > variable and check it directly in the kprobe handlers. Reviewed-by: Steven Rostedt (Google) Thanks, -- Steve
[PATCH v3] kprobe/ftrace: bail out if ftrace was killed
If an error happens in ftrace, ftrace_kill() will prevent disarming kprobes. Eventually, the ftrace_ops associated with the kprobes will be freed, yet the kprobes will still be active, and when triggered, they will use the freed memory, likely resulting in a page fault and panic. This behavior can be reproduced quite easily, by creating a kprobe and then triggering a ftrace_kill(). For simplicity, we can simulate an ftrace error with a kernel module like [1]: [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer sudo perf probe --add commit_creds sudo perf trace -e probe:commit_creds # In another terminal make sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug # Back to perf terminal # ctrl-c sudo perf probe --del commit_creds After a short period, a page fault and panic would occur as the kprobe continues to execute and uses the freed ftrace_ops. While ftrace_kill() is supposed to be used only in extreme circumstances, it is invoked in FTRACE_WARN_ON() and so there are many places where an unexpected bug could be triggered, yet the system may continue operating, possibly without the administrator noticing. If ftrace_kill() does not panic the system, then we should do everything we can to continue operating, rather than leave a ticking time bomb. Signed-off-by: Stephen Brennan --- Changes in v3: Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" variable and check it directly in the kprobe handlers. Link to v1/v2 discussion: https://lore.kernel.org/all/20240426225834.993353-1-stephen.s.bren...@oracle.com/ arch/csky/kernel/probes/ftrace.c | 3 +++ arch/loongarch/kernel/ftrace_dyn.c | 3 +++ arch/parisc/kernel/ftrace.c | 3 +++ arch/powerpc/kernel/kprobes-ftrace.c | 3 +++ arch/riscv/kernel/probes/ftrace.c| 3 +++ arch/s390/kernel/ftrace.c| 3 +++ arch/x86/kernel/kprobes/ftrace.c | 3 +++ include/linux/kprobes.h | 7 +++ kernel/kprobes.c | 6 ++ kernel/trace/ftrace.c| 1 + 10 files changed, 35 insertions(+) diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c index 834cffcfbce3..7ba4b98076de 100644 --- a/arch/csky/kernel/probes/ftrace.c +++ b/arch/csky/kernel/probes/ftrace.c @@ -12,6 +12,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, struct kprobe_ctlblk *kcb; struct pt_regs *regs; + if (unlikely(kprobe_ftrace_disabled)) + return; + bit = ftrace_test_recursion_trylock(ip, parent_ip); if (bit < 0) return; diff --git a/arch/loongarch/kernel/ftrace_dyn.c b/arch/loongarch/kernel/ftrace_dyn.c index 73858c9029cc..bff058317062 100644 --- a/arch/loongarch/kernel/ftrace_dyn.c +++ b/arch/loongarch/kernel/ftrace_dyn.c @@ -287,6 +287,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, struct kprobe *p; struct kprobe_ctlblk *kcb; + if (unlikely(kprobe_ftrace_disabled)) + return; + bit = ftrace_test_recursion_trylock(ip, parent_ip); if (bit < 0) return; diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c index 621a4b386ae4..c91f9c2e61ed 100644 --- a/arch/parisc/kernel/ftrace.c +++ b/arch/parisc/kernel/ftrace.c @@ -206,6 +206,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, struct kprobe *p; int bit; + if (unlikely(kprobe_ftrace_disabled)) + return; + bit = ftrace_test_recursion_trylock(ip, parent_ip); if (bit < 0) return; diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c index 072ebe7f290b..f8208c027148 100644 --- a/arch/powerpc/kernel/kprobes-ftrace.c +++ b/arch/powerpc/kernel/kprobes-ftrace.c @@ -21,6 +21,9 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip, struct pt_regs *regs; int bit; + if (unlikely(kprobe_ftrace_disabled)) + return; + bit = ftrace_test_recursion_trylock(nip, parent_nip); if (bit < 0) return; diff --git a/arch/riscv/kernel/probes/ftrace.c b/arch/riscv/kernel/probes/ftrace.c index 7142ec42e889..a69dfa610aa8 100644 --- a/arch/riscv/kernel/probes/ftrace.c +++ b/arch/riscv/kernel/probes/ftrace.c @@ -11,6 +11,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, struct kprobe_ctlblk *kcb; int bit; + if (unlikely(kprobe_ftrace_disabled)) + return; + bit = ftrace_test_recursion_trylock(ip, parent_ip); if (bit < 0) return; diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c index c46381ea04ec..7f6f8c438c26 100644 --- a/arch/s390/kernel/ftrace.c +++ b/arch/s390/kernel/ftrace.c @@ -296,6 +296,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
