Message-Id: <20220430011143.81fbf5ad78bae43753aa2af4@kernel.org>
Date:   Sat, 30 Apr 2022 01:11:43 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Shubham Bansal <illusionist.neo@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
        kernel-team@...com, Jiri Olsa <jolsa@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "Naveen N . Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, Will Deacon <will@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Catalin Marinas <catalin.marinas@....com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH bpf v2 0/4] kprobes: rethook,ARM,arm64: Replace
 kretprobe trampoline with rethook

Hi Mark,

Sorry for replying this late.

On Tue, 12 Apr 2022 19:03:53 +0100
Mark Rutland <mark.rutland@....com> wrote:

> On Fri, Apr 08, 2022 at 09:50:35AM +0900, Masami Hiramatsu wrote:
> > Hi,
> 
> Hi,
> 
> > Here is the 2nd version of the series for replacing kretprobe trampoline
> > with rethook on ARM/arm64. I fixed some compiler warnings in this version.
> 
> What tree is this based on? It doesn't cleanly apply atop v5.18-rc1:

I worked on bpf tree, but I'll try rebasing on the latest linus tree
(or arm/arm64 tree) in the next version.

> 
> | [mark@...rids:~/src/linux]% git am v2_20220408_mhiramat_kprobes_rethook_arm_arm64_replace_kretprobe_trampoline_with_rethook.mbx
> | Applying: ARM: unwind: Initialize the lr_addr field of unwind_ctrl_block
> | Applying: rethook,fprobe,kprobes: Check a failure in the rethook_hook()
> | Applying: ARM: rethook: Replace kretprobe trampoline with rethook
> | error: patch failed: arch/arm/kernel/stacktrace.c:66
> | error: arch/arm/kernel/stacktrace.c: patch does not apply
> | Patch failed at 0003 ARM: rethook: Replace kretprobe trampoline with rethook
> | hint: Use 'git am --show-current-patch=diff' to see the failed patch
> | When you have resolved this problem, run "git am --continue".
> | If you prefer to skip this patch, run "git am --skip" instead.
> | To restore the original branch and stop patching, run "git am --abort".
> 
> I've done a `git am -3` locally to make that work for now.
> 
> > The previous version is here[1];
> > 
> > [1] https://lore.kernel.org/all/164915121498.982637.12787715964983738566.stgit@devnote2/T/#u
> > 
> > This series includes a trivial bugfix for the arm unwinder to initialize
> > an internal data structure([1/4]). This is not critical for stack trace,
> > but required for rethook to find the LR register from the stack.
> > This also have an update for the rethook interface, which allows us to
> > check the rethook_hook() failure ([2/4]). This is also required for the
> > rethook on arm because unwinder is able to fail.
> > The rest of patches are replacing kretprobe trampoline with rethook on
> > ARM ([3/4]) and arm64 ([4/4]).
> 
> Generally, the arm and arm64 bits go via different trees, and for unwinding the
> two are quite different.

OK, Should I split this into 2 series?
Anyway, I'll send the first one for arm tree because this is a real bug.

> 
> IIUC the dependency between the two is just because patch 2 changes the
> prototypes of some functions. Is that right?

Yes. I can push it one by one. What is the best way, would you think?
If I split this into some series, I will;

- Send [1/4] to arm tree as a bugfix.
- Send [2/4] and [3/4] to arm tree as an enhancement.
- Send [4/4] to arm64 when above 2 series are merged.

Is that good for you?

> 
> 
> > Background:
> > 
> > This rethook came from Jiri's request of multiple kprobe for bpf[2].
> > He tried to solve an issue that starting bpf with multiple kprobe will
> > take a long time because bpf-kprobe will wait for RCU grace period for
> > sync rcu events.
> > 
> > Jiri wanted to attach a single bpf handler to multiple kprobes and
> > he tried to introduce multiple-probe interface to kprobe. So I asked
> > him to use ftrace and kretprobe-like hook if it is only for the
> > function entry and exit, instead of adding ad-hoc interface
> > to kprobes.
> > For this purpose, I introduced the fprobe (kprobe like interface for
> > ftrace) with the rethook (this is a generic return hook feature for
> > fprobe exit handler)[3].
> > 
> > [2] https://lore.kernel.org/all/20220104080943.113249-1-jolsa@kernel.org/T/#u
> > [3] https://lore.kernel.org/all/164191321766.806991.7930388561276940676.stgit@devnote2/T/#u
> > 
> > The rethook is basically same as the kretprobe trampoline. I just made
> > it decoupled from kprobes. Eventually, the all arch dependent kretprobe
> > trampolines will be replaced with the rethook trampoline instead of
> > cloning the code.
> > 
> > When I port the rethook for all arch which supports kretprobe, the
> > legacy kretprobe specific code (which is for CONFIG_KRETPROBE_ON_RETHOOK=n)
> > will be removed eventually.
> > 
> > BTW, the arm Clang support for rethook is for kretprobes only. fprobe
> > and ftrace seems not working with Clang yet.
> 
> Do you mean that's an existing issue?

Yes, but the above is not enough, Clang on arm and dynamic ftrace doesn't work.
Perhaps, Clang on arm doesn't support -mrecord-mcount? 

Thank you,

> 
> Thanks,
> Mark.
> 
> > 
> > Thank you,
> > 
> > ---
> > 
> > Masami Hiramatsu (4):
> >       ARM: unwind: Initialize the lr_addr field of unwind_ctrl_block
> >       rethook,fprobe,kprobes: Check a failure in the rethook_hook()
> >       ARM: rethook: Replace kretprobe trampoline with rethook
> >       arm64: rethook: Replace kretprobe trampoline with rethook
> > 
> > 
> >  arch/arm/Kconfig                              |    1 
> >  arch/arm/include/asm/stacktrace.h             |    5 +
> >  arch/arm/kernel/stacktrace.c                  |   13 +--
> >  arch/arm/kernel/unwind.c                      |    1 
> >  arch/arm/probes/Makefile                      |    1 
> >  arch/arm/probes/kprobes/core.c                |   62 ------------
> >  arch/arm/probes/rethook.c                     |  127 +++++++++++++++++++++++++
> >  arch/arm64/Kconfig                            |    1 
> >  arch/arm64/include/asm/kprobes.h              |    2 
> >  arch/arm64/include/asm/stacktrace.h           |    2 
> >  arch/arm64/kernel/Makefile                    |    1 
> >  arch/arm64/kernel/probes/Makefile             |    1 
> >  arch/arm64/kernel/probes/kprobes.c            |   15 ---
> >  arch/arm64/kernel/probes/kprobes_trampoline.S |   86 -----------------
> >  arch/arm64/kernel/rethook.c                   |   28 ++++++
> >  arch/arm64/kernel/rethook_trampoline.S        |   87 +++++++++++++++++
> >  arch/arm64/kernel/stacktrace.c                |    9 +-
> >  arch/x86/kernel/rethook.c                     |    4 +
> >  include/linux/rethook.h                       |    4 -
> >  kernel/kprobes.c                              |    8 +-
> >  kernel/trace/fprobe.c                         |    5 +
> >  kernel/trace/rethook.c                        |   12 ++
> >  22 files changed, 287 insertions(+), 188 deletions(-)
> >  create mode 100644 arch/arm/probes/rethook.c
> >  delete mode 100644 arch/arm64/kernel/probes/kprobes_trampoline.S
> >  create mode 100644 arch/arm64/kernel/rethook.c
> >  create mode 100644 arch/arm64/kernel/rethook_trampoline.S
> > 
> > --
> > Masami Hiramatsu (Linaro) <mhiramat@...nel.org>


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists