mbox series

[bpf-next,v2,0/4] kprobes: rethook: x86: Replace kretprobe trampoline with rethook

Message ID 164821817332.2373735.12048266953420821089.stgit@devnote2 (mailing list archive)
Headers show
Series kprobes: rethook: x86: Replace kretprobe trampoline with rethook | expand

Message

Masami Hiramatsu (Google) March 25, 2022, 2:22 p.m. UTC
Hi,

Here are the 2nd version for generic kretprobe and kretprobe on x86 for
replacing the kretprobe trampoline with rethook. The previous version
is here[1]

[1] https://lore.kernel.org/all/164818251899.2252200.7306353689206167903.stgit@devnote2/T/#u

In this version I added completing pt_regs by saving regs->ss register
for rethook (from Peter) and optprobe.

Background:

This rethook came from Jiri's request of multiple kprobe for bpf[1].
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)[2].

[1] https://lore.kernel.org/all/20220104080943.113249-1-jolsa@kernel.org/T/#u
[2] 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 and set HAVE_RETHOOK=y.
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.

Worktree notice:

BTW, this patch can be applied to next-20220324, not the bpf-next tree
directly, because this depends on ANNOTATE_NOENDBR macro. However, since
the fprobe is merged in the bpf-next, I marked this for bpf-next.
So until merging the both of fprobes and ENDBR series, to compile this
you need below 2 lines in arch/x86/kernel/rethook.c.

#ifndef ANNOTATE_NOENDBR
#define ANNOTATE_NOENDBR

But after those are merged, these lines will be unneeded. How should I
handle this issue? (Just remove ANNOTATE_NOENDBR line in bpf-next?)


Thank you,

---

Masami Hiramatsu (3):
      kprobes: Use rethook for kretprobe if possible
      rethook: kprobes: x86: Replace kretprobe with rethook on x86
      x86,kprobes: Fix optprobe trampoline to generate complete pt_regs

Peter Zijlstra (1):
      Subject: x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs


 arch/Kconfig                     |    7 ++
 arch/x86/Kconfig                 |    1 
 arch/x86/include/asm/unwind.h    |   23 +++----
 arch/x86/kernel/Makefile         |    1 
 arch/x86/kernel/kprobes/common.h |    1 
 arch/x86/kernel/kprobes/core.c   |  107 ---------------------------------
 arch/x86/kernel/kprobes/opt.c    |   25 +++++---
 arch/x86/kernel/rethook.c        |  123 ++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/unwind_orc.c     |   10 ++-
 include/linux/kprobes.h          |   51 +++++++++++++++-
 kernel/kprobes.c                 |  124 ++++++++++++++++++++++++++++++++------
 kernel/trace/trace_kprobe.c      |    4 +
 12 files changed, 319 insertions(+), 158 deletions(-)
 create mode 100644 arch/x86/kernel/rethook.c

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

Comments

Peter Zijlstra March 25, 2022, 2:43 p.m. UTC | #1
On Fri, Mar 25, 2022 at 11:22:53PM +0900, Masami Hiramatsu wrote:

> Masami Hiramatsu (3):
>       kprobes: Use rethook for kretprobe if possible
>       rethook: kprobes: x86: Replace kretprobe with rethook on x86
>       x86,kprobes: Fix optprobe trampoline to generate complete pt_regs
> 
> Peter Zijlstra (1):
>       Subject: x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs

You fat-fingered the subject there ^

Other than that:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Hopefully the ftrace return trampoline can also be switched over..
Alexei Starovoitov March 25, 2022, 4:49 p.m. UTC | #2
On Fri, Mar 25, 2022 at 7:43 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Mar 25, 2022 at 11:22:53PM +0900, Masami Hiramatsu wrote:
>
> > Masami Hiramatsu (3):
> >       kprobes: Use rethook for kretprobe if possible
> >       rethook: kprobes: x86: Replace kretprobe with rethook on x86
> >       x86,kprobes: Fix optprobe trampoline to generate complete pt_regs
> >
> > Peter Zijlstra (1):
> >       Subject: x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs
>
> You fat-fingered the subject there ^
>
> Other than that:
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> Hopefully the ftrace return trampoline can also be switched over..

Thanks Peter. What's an ETA on landing endbr set?
Did I miss a pull req?
I see an odd error in linux-next with bpf selftests
which may or may not be related. Planning to debug it
when everything settles in Linus's tree.

Masami, could you do another respin?

Also do you mind squashing patches 2,3,4 ?
It's odd to have the same lines of code patched up 3 times.
Just do it right once.
Peter Zijlstra March 25, 2022, 4:51 p.m. UTC | #3
On Fri, Mar 25, 2022 at 03:43:15PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 25, 2022 at 11:22:53PM +0900, Masami Hiramatsu wrote:
> 
> > Masami Hiramatsu (3):
> >       kprobes: Use rethook for kretprobe if possible
> >       rethook: kprobes: x86: Replace kretprobe with rethook on x86
> >       x86,kprobes: Fix optprobe trampoline to generate complete pt_regs
> > 
> > Peter Zijlstra (1):
> >       Subject: x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs
> 
> You fat-fingered the subject there ^
> 
> Other than that:
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Hopefully the ftrace return trampoline can also be switched over..

Urgh, allnoconfig doesn't build because..

diff --git a/kernel/Makefile b/kernel/Makefile
index 56f4ee97f328..471d71935e90 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -108,6 +108,7 @@ obj-$(CONFIG_TRACING) += trace/
 obj-$(CONFIG_TRACE_CLOCK) += trace/
 obj-$(CONFIG_RING_BUFFER) += trace/
 obj-$(CONFIG_TRACEPOINTS) += trace/
+obj-$(CONFIG_RETHOOK) += trace/
 obj-$(CONFIG_IRQ_WORK) += irq_work.o
 obj-$(CONFIG_CPU_PM) += cpu_pm.o
 obj-$(CONFIG_BPF) += bpf/
Masami Hiramatsu (Google) March 26, 2022, 1:09 a.m. UTC | #4
On Fri, 25 Mar 2022 15:43:14 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Mar 25, 2022 at 11:22:53PM +0900, Masami Hiramatsu wrote:
> 
> > Masami Hiramatsu (3):
> >       kprobes: Use rethook for kretprobe if possible
> >       rethook: kprobes: x86: Replace kretprobe with rethook on x86
> >       x86,kprobes: Fix optprobe trampoline to generate complete pt_regs
> > 
> > Peter Zijlstra (1):
> >       Subject: x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs
> 
> You fat-fingered the subject there ^
> 

Oops, I missed to import the patch...

> Other than that:
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Thanks!

> 
> Hopefully the ftrace return trampoline can also be switched over..

The rethook clarifies the interfaces for the return trampoline, so
I think this can step the integration forward.
Masami Hiramatsu (Google) March 26, 2022, 1:20 a.m. UTC | #5
On Fri, 25 Mar 2022 17:51:19 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Mar 25, 2022 at 03:43:15PM +0100, Peter Zijlstra wrote:
> > On Fri, Mar 25, 2022 at 11:22:53PM +0900, Masami Hiramatsu wrote:
> > 
> > > Masami Hiramatsu (3):
> > >       kprobes: Use rethook for kretprobe if possible
> > >       rethook: kprobes: x86: Replace kretprobe with rethook on x86
> > >       x86,kprobes: Fix optprobe trampoline to generate complete pt_regs
> > > 
> > > Peter Zijlstra (1):
> > >       Subject: x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs
> > 
> > You fat-fingered the subject there ^
> > 
> > Other than that:
> > 
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > 
> > Hopefully the ftrace return trampoline can also be switched over..
> 
> Urgh, allnoconfig doesn't build because..
> 
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 56f4ee97f328..471d71935e90 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -108,6 +108,7 @@ obj-$(CONFIG_TRACING) += trace/
>  obj-$(CONFIG_TRACE_CLOCK) += trace/
>  obj-$(CONFIG_RING_BUFFER) += trace/
>  obj-$(CONFIG_TRACEPOINTS) += trace/
> +obj-$(CONFIG_RETHOOK) += trace/
>  obj-$(CONFIG_IRQ_WORK) += irq_work.o
>  obj-$(CONFIG_CPU_PM) += cpu_pm.o
>  obj-$(CONFIG_BPF) += bpf/

Oops, thanks for pointing out!
Masami Hiramatsu (Google) March 26, 2022, 1:26 a.m. UTC | #6
On Fri, 25 Mar 2022 09:49:47 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Fri, Mar 25, 2022 at 7:43 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Fri, Mar 25, 2022 at 11:22:53PM +0900, Masami Hiramatsu wrote:
> >
> > > Masami Hiramatsu (3):
> > >       kprobes: Use rethook for kretprobe if possible
> > >       rethook: kprobes: x86: Replace kretprobe with rethook on x86
> > >       x86,kprobes: Fix optprobe trampoline to generate complete pt_regs
> > >
> > > Peter Zijlstra (1):
> > >       Subject: x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs
> >
> > You fat-fingered the subject there ^
> >
> > Other than that:
> >
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> >
> > Hopefully the ftrace return trampoline can also be switched over..
> 
> Thanks Peter. What's an ETA on landing endbr set?
> Did I miss a pull req?
> I see an odd error in linux-next with bpf selftests
> which may or may not be related. Planning to debug it
> when everything settles in Linus's tree.

That is what I pointed in cover mail.

> BTW, this patch can be applied to next-20220324, not the bpf-next tree
> directly, because this depends on ANNOTATE_NOENDBR macro. However, since
> the fprobe is merged in the bpf-next, I marked this for bpf-next.
> So until merging the both of fprobes and ENDBR series, to compile this
> you need below 2 lines in arch/x86/kernel/rethook.c.
> 
> #ifndef ANNOTATE_NOENDBR
> #define ANNOTATE_NOENDBR

> 
> Masami, could you do another respin?

OK, I will add above temporary mitigation.

> 
> Also do you mind squashing patches 2,3,4 ?
> It's odd to have the same lines of code patched up 3 times.
> Just do it right once.

Hmm, I think those are different commit for different features.
I would like to keep those 3 patches separated (for the case if
we find any issue to introduce regs->ss later)

Thank you,