Re: [PATCH v2] kprobe/ftrace: bail out if ftrace was killed
Steven Rostedt writes: > On Mon, 29 Apr 2024 10:47:18 -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 >> --- >> Difference from v1: removed both existing declarations of ftrace_is_dead() >> from kernel/trace/trace.h. >> >> 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/ftrace.h | 2 ++ >> kernel/trace/trace.h | 2 -- >> 9 files changed, 23 insertions(+), 2 deletions(-) >> >> diff --git a/arch/csky/kernel/probes/ftrace.c >> b/arch/csky/kernel/probes/ftrace.c >> index 834cffcfbce3..3931bf9f707b 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(ftrace_is_dead())) >> +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..82c952cb5be0 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(ftrace_is_dead())) >> +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..3660834f54c3 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(ftrace_is_dead())) >> +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..85eb55aa1457 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(ftrace_is_dead())) >> +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..8814fbe4c888 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(ftrace_is_dead())) >> +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..ccbe8ccf945b 100644 >> ---
Re: [PATCH v2] kprobe/ftrace: bail out if ftrace was killed
On Mon, 29 Apr 2024 10:47:18 -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 > --- > Difference from v1: removed both existing declarations of ftrace_is_dead() > from kernel/trace/trace.h. > > 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/ftrace.h | 2 ++ > kernel/trace/trace.h | 2 -- > 9 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/arch/csky/kernel/probes/ftrace.c > b/arch/csky/kernel/probes/ftrace.c > index 834cffcfbce3..3931bf9f707b 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(ftrace_is_dead())) > + 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..82c952cb5be0 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(ftrace_is_dead())) > + 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..3660834f54c3 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(ftrace_is_dead())) > + 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..85eb55aa1457 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(ftrace_is_dead())) > + 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..8814fbe4c888 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(ftrace_is_dead())) > + 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..ccbe8ccf945b 100644 > --- a/arch/s390/kernel/ftrace.c > +++ b/arch/s390/kernel/ftrace.c > @@ -296,6 +296,9 @@ void kprobe_ftrace_handler(unsigned long
[PATCH v2] 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 --- Difference from v1: removed both existing declarations of ftrace_is_dead() from kernel/trace/trace.h. 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/ftrace.h | 2 ++ kernel/trace/trace.h | 2 -- 9 files changed, 23 insertions(+), 2 deletions(-) diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c index 834cffcfbce3..3931bf9f707b 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(ftrace_is_dead())) + 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..82c952cb5be0 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(ftrace_is_dead())) + 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..3660834f54c3 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(ftrace_is_dead())) + 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..85eb55aa1457 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(ftrace_is_dead())) + 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..8814fbe4c888 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(ftrace_is_dead())) + 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..ccbe8ccf945b 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, struct kprobe *p; int bit; + if (unlikely(ftrace_is_dead())) + return; + bit = ftrace_test_recursion_trylock(ip, parent_ip); if (bit < 0) return;