Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed

On Wed, 15 May 2024 15:18:08 -0700
Stephen Brennan [off-list ref] wrote:

Masami Hiramatsu (Google) [off-list ref] writes:

quoted

On Thu, 2 May 2024 01:35:16 +0800
Guo Ren [off-list ref] wrote:

quoted

On Thu, May 2, 2024 at 12:30 AM Stephen Brennan
[off-list ref] wrote:

quoted

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 <redacted>
---
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.brennan@oracle.com/ (local)

 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 <guoren@kernel.org>

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) [off-list ref]