[RFC PATCH v2 0/6] bpf: fprobe: rethook: Use ftrace_regs instead of pt_regs

[RFC PATCH v2 0/6] bpf: fprobe: rethook: Use ftrace_regs instead of pt_regs

[Masami Hiramatsu (Google)]

Hi,

Here is the 2nd version of RFC series to use ftrace_regs instead of pt_regs.
But this includes the generic part and minimum modifications of arch
dependent code. (e.g. not including rethook for arm64.) This series is based
on the discussion at

https://lore.kernel.org/all/20230801112036.0d4ee60d@gandalf.local.home/

This version includes 1 patch to expose ftrace_regs. so

 - Simply replace pt_regs in fprobe_entry_handler with ftrace_regs.
 - Expose ftrace_regs even if CONFIG_FUNCTION_TRACER=n.
 - Replace pt_regs in rethook and fprobe_exit_handler with ftrace_regs. This
   introduce a new HAVE_PT_REGS_COMPAT_FTRACE_REGS which means ftrace_regs is
   just a wrapper of pt_regs (except for arm64, other architectures do this)
 - Update fprobe-events to use ftrace_regs natively.
 - Introduce ftrace_partial_regs(). (This changes ARM64 which needs a custom
   implementation)
 - Update bpf multi-kprobe handler use ftrace_partial_regs().

Florent, feel free to add your rethook for arm64, but please do not remove
kretprobe trampoline yet. It is another discussion point. We may be possible
to use ftrace_regs for kretprobe by ftrace_partial_regs() but kretprobe
allows nest probe. (maybe we can skip that case?)

This series can also be found below branch.

https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/log/?h=topic/fprobe-ftrace-regs

Thank you,

---

Masami Hiramatsu (Google) (6):
      fprobe: Use fprobe_regs in fprobe entry handler
      tracing: Expose ftrace_regs regardless of CONFIG_FUNCTION_TRACER
      fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook
      tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS
      ftrace: Add ftrace_partial_regs() for converting ftrace_regs to pt_regs
      bpf: Enable kprobe_multi feature if CONFIG_FPROBE is enabled


 arch/Kconfig                    |    1 +
 arch/arm64/include/asm/ftrace.h |   11 ++++++
 arch/loongarch/Kconfig          |    1 +
 arch/s390/Kconfig               |    1 +
 arch/x86/Kconfig                |    1 +
 arch/x86/kernel/rethook.c       |    9 +++--
 include/linux/fprobe.h          |    4 +-
 include/linux/ftrace.h          |   56 ++++++++++++++++++-----------
 include/linux/rethook.h         |   11 +++---
 kernel/kprobes.c                |    9 ++++-
 kernel/trace/Kconfig            |    9 ++++-
 kernel/trace/bpf_trace.c        |   14 +++++--
 kernel/trace/fprobe.c           |    8 ++--
 kernel/trace/rethook.c          |   16 ++++----
 kernel/trace/trace_fprobe.c     |   76 ++++++++++++++++++++++++---------------
 kernel/trace/trace_probe_tmpl.h |    2 +
 lib/test_fprobe.c               |   10 +++--
 samples/fprobe/fprobe_example.c |    4 +-
 18 files changed, 154 insertions(+), 89 deletions(-)

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

[RFC PATCH v2 1/6] fprobe: Use fprobe_regs in fprobe entry handler

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

This allows fprobes to be available with CONFIG_DYNAMIC_FTRACE_WITH_ARGS
instead of CONFIG_DYNAMIC_FTRACE_WITH_REGS, then we can enable fprobe
on arm64.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/fprobe.h          |    2 +-
 kernel/trace/Kconfig            |    3 ++-
 kernel/trace/bpf_trace.c        |   10 +++++++---
 kernel/trace/fprobe.c           |    2 +-
 kernel/trace/trace_fprobe.c     |    6 +++++-
 lib/test_fprobe.c               |    4 ++--
 samples/fprobe/fprobe_example.c |    2 +-
 7 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
index 3e03758151f4..36c0595f7b93 100644
--- a/include/linux/fprobe.h
+++ b/include/linux/fprobe.h
@@ -35,7 +35,7 @@ struct fprobe {
 	int			nr_maxactive;
 
 	int (*entry_handler)(struct fprobe *fp, unsigned long entry_ip,
-			     unsigned long ret_ip, struct pt_regs *regs,
+			     unsigned long ret_ip, struct ftrace_regs *regs,
 			     void *entry_data);
 	void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
 			     unsigned long ret_ip, struct pt_regs *regs,
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 61c541c36596..976fd594b446 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -287,7 +287,7 @@ config DYNAMIC_FTRACE_WITH_ARGS
 config FPROBE
 	bool "Kernel Function Probe (fprobe)"
 	depends on FUNCTION_TRACER
-	depends on DYNAMIC_FTRACE_WITH_REGS
+	depends on DYNAMIC_FTRACE_WITH_REGS || DYNAMIC_FTRACE_WITH_ARGS
 	depends on HAVE_RETHOOK
 	select RETHOOK
 	default n
@@ -672,6 +672,7 @@ config FPROBE_EVENTS
 	select TRACING
 	select PROBE_EVENTS
 	select DYNAMIC_EVENTS
+	depends on DYNAMIC_FTRACE_WITH_REGS
 	default y
 	help
 	  This allows user to add tracing events on the function entry and
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 5f2dcabad202..51573eaa04c4 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2460,7 +2460,7 @@ static int __init bpf_event_init(void)
 fs_initcall(bpf_event_init);
 #endif /* CONFIG_MODULES */
 
-#ifdef CONFIG_FPROBE
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 struct bpf_kprobe_multi_link {
 	struct bpf_link link;
 	struct fprobe fp;
@@ -2652,10 +2652,14 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 
 static int
 kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
-			  unsigned long ret_ip, struct pt_regs *regs,
+			  unsigned long ret_ip, struct ftrace_regs *fregs,
 			  void *data)
 {
 	struct bpf_kprobe_multi_link *link;
+	struct pt_regs *regs = ftrace_get_regs(fregs);
+
+	if (!regs)
+		return 0;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
 	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
@@ -2910,7 +2914,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	kvfree(cookies);
 	return err;
 }
-#else /* !CONFIG_FPROBE */
+#else /* !CONFIG_DYNAMIC_FTRACE_WITH_REGS */
 int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
 	return -EOPNOTSUPP;
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index 3b21f4063258..15a2aef92733 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -46,7 +46,7 @@ static inline void __fprobe_handler(unsigned long ip, unsigned long parent_ip,
 	}
 
 	if (fp->entry_handler)
-		ret = fp->entry_handler(fp, ip, parent_ip, ftrace_get_regs(fregs), entry_data);
+		ret = fp->entry_handler(fp, ip, parent_ip, fregs, entry_data);
 
 	/* If entry_handler returns !0, nmissed is not counted. */
 	if (rh) {
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index dfe2e546acdc..4d3ae79f036e 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -320,12 +320,16 @@ NOKPROBE_SYMBOL(fexit_perf_func);
 #endif	/* CONFIG_PERF_EVENTS */
 
 static int fentry_dispatcher(struct fprobe *fp, unsigned long entry_ip,
-			     unsigned long ret_ip, struct pt_regs *regs,
+			     unsigned long ret_ip, struct ftrace_regs *fregs,
 			     void *entry_data)
 {
 	struct trace_fprobe *tf = container_of(fp, struct trace_fprobe, fp);
+	struct pt_regs *regs = ftrace_get_regs(fregs);
 	int ret = 0;
 
+	if (!regs)
+		return 0;
+
 	if (trace_probe_test_flag(&tf->tp, TP_FLAG_TRACE))
 		fentry_trace_func(tf, entry_ip, regs);
 #ifdef CONFIG_PERF_EVENTS
diff --git a/lib/test_fprobe.c b/lib/test_fprobe.c
index 24de0e5ff859..ff607babba18 100644
--- a/lib/test_fprobe.c
+++ b/lib/test_fprobe.c
@@ -40,7 +40,7 @@ static noinline u32 fprobe_selftest_nest_target(u32 value, u32 (*nest)(u32))
 
 static notrace int fp_entry_handler(struct fprobe *fp, unsigned long ip,
 				    unsigned long ret_ip,
-				    struct pt_regs *regs, void *data)
+				    struct ftrace_regs *fregs, void *data)
 {
 	KUNIT_EXPECT_FALSE(current_test, preemptible());
 	/* This can be called on the fprobe_selftest_target and the fprobe_selftest_target2 */
@@ -81,7 +81,7 @@ static notrace void fp_exit_handler(struct fprobe *fp, unsigned long ip,
 
 static notrace int nest_entry_handler(struct fprobe *fp, unsigned long ip,
 				      unsigned long ret_ip,
-				      struct pt_regs *regs, void *data)
+				      struct ftrace_regs *fregs, void *data)
 {
 	KUNIT_EXPECT_FALSE(current_test, preemptible());
 	return 0;
diff --git a/samples/fprobe/fprobe_example.c b/samples/fprobe/fprobe_example.c
index 64e715e7ed11..1545a1aac616 100644
--- a/samples/fprobe/fprobe_example.c
+++ b/samples/fprobe/fprobe_example.c
@@ -50,7 +50,7 @@ static void show_backtrace(void)
 
 static int sample_entry_handler(struct fprobe *fp, unsigned long ip,
 				unsigned long ret_ip,
-				struct pt_regs *regs, void *data)
+				struct ftrace_regs *fregs, void *data)
 {
 	if (use_trace)
 		/*

Re: [RFC PATCH v2 1/6] fprobe: Use fprobe_regs in fprobe entry handler

[Florent Revest]

On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> This allows fprobes to be available with CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> instead of CONFIG_DYNAMIC_FTRACE_WITH_REGS, then we can enable fprobe
> on arm64.

This patch lets fprobe code build on configs WITH_ARGS and !WITH_REGS
but fprobe wouldn't run on these builds because fprobe still registers
to ftrace with FTRACE_OPS_FL_SAVE_REGS, which would fail on
!WITH_REGS. Shouldn't we also let the fprobe_init callers decide
whether they want REGS or not ?

>  static int
>  kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
> -                         unsigned long ret_ip, struct pt_regs *regs,
> +                         unsigned long ret_ip, struct ftrace_regs *fregs,
>                           void *data)
>  {
>         struct bpf_kprobe_multi_link *link;
> +       struct pt_regs *regs = ftrace_get_regs(fregs);
> +
> +       if (!regs)
> +               return 0;

(with the above comment addressed) this means that BPF multi_kprobe
would successfully attach on builds WITH_ARGS but the programs would
never actually run because here regs would be 0. This is a confusing
failure mode for the user. I think that if multi_kprobe won't work
(because we don't have a pt_regs conversion path yet), the user should
be notified at attachment time that they won't be getting any events.
That's why I think kprobe_multi should inform fprobe_init that it
wants FTRACE_OPS_FL_SAVE_REGS and fail if that's not possible (no
trampoline for it for example)

Re: [RFC PATCH v2 1/6] fprobe: Use fprobe_regs in fprobe entry handler

[Masami Hiramatsu]

Hi Florent,

On Wed, 9 Aug 2023 12:28:38 +0200
Florent Revest <revest@chromium.org> wrote:

> On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > This allows fprobes to be available with CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > instead of CONFIG_DYNAMIC_FTRACE_WITH_REGS, then we can enable fprobe
> > on arm64.
> 
> This patch lets fprobe code build on configs WITH_ARGS and !WITH_REGS
> but fprobe wouldn't run on these builds because fprobe still registers
> to ftrace with FTRACE_OPS_FL_SAVE_REGS, which would fail on
> !WITH_REGS. Shouldn't we also let the fprobe_init callers decide
> whether they want REGS or not ?

Ah, I think you meant FPROBE_EVENTS? Yes I forgot to add the dependency
on it. But fprobe itself can work because fprobe just pass the ftrace_regs
to the handlers. (Note that exit callback may not work until next patch)

> 
> >  static int
> >  kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
> > -                         unsigned long ret_ip, struct pt_regs *regs,
> > +                         unsigned long ret_ip, struct ftrace_regs *fregs,
> >                           void *data)
> >  {
> >         struct bpf_kprobe_multi_link *link;
> > +       struct pt_regs *regs = ftrace_get_regs(fregs);
> > +
> > +       if (!regs)
> > +               return 0;
> 
> (with the above comment addressed) this means that BPF multi_kprobe
> would successfully attach on builds WITH_ARGS but the programs would
> never actually run because here regs would be 0. This is a confusing
> failure mode for the user. I think that if multi_kprobe won't work
> (because we don't have a pt_regs conversion path yet), the user should
> be notified at attachment time that they won't be getting any events.

Yes, so I changed it will not be compiled in that case.

@@ -2460,7 +2460,7 @@ static int __init bpf_event_init(void)
 fs_initcall(bpf_event_init);
 #endif /* CONFIG_MODULES */
 
-#ifdef CONFIG_FPROBE
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 struct bpf_kprobe_multi_link {
 	struct bpf_link link;
 	struct fprobe fp;

> That's why I think kprobe_multi should inform fprobe_init that it
> wants FTRACE_OPS_FL_SAVE_REGS and fail if that's not possible (no
> trampoline for it for example)

Yeah, that's another possibility, but in the previous thread we
discussed and agreed to introduce the ftrace_partial_regs() which
will copy the partial registers from ftrace_regs to pt_regs.


Thank you,


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

Re: [RFC PATCH v2 1/6] fprobe: Use fprobe_regs in fprobe entry handler

[Florent Revest]

On Wed, Aug 9, 2023 at 4:10 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Florent,
>
> On Wed, 9 Aug 2023 12:28:38 +0200
> Florent Revest <revest@chromium.org> wrote:
>
> > On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
> > <mhiramat@kernel.org> wrote:
> > >
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > >
> > > This allows fprobes to be available with CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > > instead of CONFIG_DYNAMIC_FTRACE_WITH_REGS, then we can enable fprobe
> > > on arm64.
> >
> > This patch lets fprobe code build on configs WITH_ARGS and !WITH_REGS
> > but fprobe wouldn't run on these builds because fprobe still registers
> > to ftrace with FTRACE_OPS_FL_SAVE_REGS, which would fail on
> > !WITH_REGS. Shouldn't we also let the fprobe_init callers decide
> > whether they want REGS or not ?
>
> Ah, I think you meant FPROBE_EVENTS? Yes I forgot to add the dependency
> on it. But fprobe itself can work because fprobe just pass the ftrace_regs
> to the handlers. (Note that exit callback may not work until next patch)

No, I mean that fprobe still registers its ftrace ops with the
FTRACE_OPS_FL_SAVE_REGS flag:

https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/kernel/trace/fprobe.c?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n185

Which means that __register_ftrace_function will return -EINVAL on
builds !WITH_REGS:

https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/kernel/trace/ftrace.c?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n338

As documented here:

https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/include/linux/ftrace.h?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n188

There are two parts to using sparse pt_regs. One is "static": having
WITH_ARGS in the config, the second one is "dynamic": a ftrace ops
needs to specify that it doesn't want to go through the ftrace
trampoline that saves a full pt_regs, by not giving
FTRACE_OPS_FL_SAVE_REGS. If we want fprobe to work on builds
!WITH_REGS then we should both remove Kconfig dependencies to
WITH_REGS (like you've done) but also stop passing this ftrace ops
flag.

> >
> > >  static int
> > >  kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
> > > -                         unsigned long ret_ip, struct pt_regs *regs,
> > > +                         unsigned long ret_ip, struct ftrace_regs *fregs,
> > >                           void *data)
> > >  {
> > >         struct bpf_kprobe_multi_link *link;
> > > +       struct pt_regs *regs = ftrace_get_regs(fregs);
> > > +
> > > +       if (!regs)
> > > +               return 0;
> >
> > (with the above comment addressed) this means that BPF multi_kprobe
> > would successfully attach on builds WITH_ARGS but the programs would
> > never actually run because here regs would be 0. This is a confusing
> > failure mode for the user. I think that if multi_kprobe won't work
> > (because we don't have a pt_regs conversion path yet), the user should
> > be notified at attachment time that they won't be getting any events.
>
> Yes, so I changed it will not be compiled in that case.

Ah ok I missed the CONFIG_DYNAMIC_FTRACE_WITH_REGS guard that you keep
between patch 1 and 6 to avoid this case. (after patch 6, it's no
longer an issue) then that's fine.

Re: [RFC PATCH v2 1/6] fprobe: Use fprobe_regs in fprobe entry handler

[Florent Revest]

On Wed, Aug 9, 2023 at 6:09 PM Florent Revest <revest@chromium.org> wrote:
>
> On Wed, Aug 9, 2023 at 4:10 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Hi Florent,
> >
> > On Wed, 9 Aug 2023 12:28:38 +0200
> > Florent Revest <revest@chromium.org> wrote:
> >
> > > On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
> > > <mhiramat@kernel.org> wrote:
> > > >
> > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > >
> > > > This allows fprobes to be available with CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > > > instead of CONFIG_DYNAMIC_FTRACE_WITH_REGS, then we can enable fprobe
> > > > on arm64.
> > >
> > > This patch lets fprobe code build on configs WITH_ARGS and !WITH_REGS
> > > but fprobe wouldn't run on these builds because fprobe still registers
> > > to ftrace with FTRACE_OPS_FL_SAVE_REGS, which would fail on
> > > !WITH_REGS. Shouldn't we also let the fprobe_init callers decide
> > > whether they want REGS or not ?
> >
> > Ah, I think you meant FPROBE_EVENTS? Yes I forgot to add the dependency
> > on it. But fprobe itself can work because fprobe just pass the ftrace_regs
> > to the handlers. (Note that exit callback may not work until next patch)
>
> No, I mean that fprobe still registers its ftrace ops with the
> FTRACE_OPS_FL_SAVE_REGS flag:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/kernel/trace/fprobe.c?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n185
>
> Which means that __register_ftrace_function will return -EINVAL on
> builds !WITH_REGS:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/kernel/trace/ftrace.c?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n338
>
> As documented here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/include/linux/ftrace.h?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n188
>
> There are two parts to using sparse pt_regs. One is "static": having
> WITH_ARGS in the config, the second one is "dynamic": a ftrace ops
> needs to specify that it doesn't want to go through the ftrace
> trampoline that saves a full pt_regs, by not giving
> FTRACE_OPS_FL_SAVE_REGS. If we want fprobe to work on builds
> !WITH_REGS then we should both remove Kconfig dependencies to
> WITH_REGS (like you've done) but also stop passing this ftrace ops
> flag.

Said in a different way: there are arches that support both WITH_ARGS
and WITH_REGS (like x86 actually). They have two ftrace trampolines
compiled in: ftrace_caller and ftrace_regs_caller, one for each
usecase. If you register to ftrace with the FTRACE_OPS_FL_SAVE_REGS
flag you are telling it that what you want is a pt_regs. If you are
trying to move away from pt_regs and support ftrace_regs in the more
general case (meaning, in the case where it can contain a sparse
pt_regs) then you should stop passing that flag so you go through the
lighter, faster trampoline and test your code in the circumstances
where ftrace_regs isn't just a regular pt_regs but an actually sparse
or light data structure.

I hope that makes my thoughts clearer? It's a hairy topic ahah

Re: [RFC PATCH v2 1/6] fprobe: Use fprobe_regs in fprobe entry handler

[Masami Hiramatsu]

On Wed, 9 Aug 2023 18:17:47 +0200
Florent Revest <revest@chromium.org> wrote:

> On Wed, Aug 9, 2023 at 6:09 PM Florent Revest <revest@chromium.org> wrote:
> >
> > On Wed, Aug 9, 2023 at 4:10 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > Hi Florent,
> > >
> > > On Wed, 9 Aug 2023 12:28:38 +0200
> > > Florent Revest <revest@chromium.org> wrote:
> > >
> > > > On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
> > > > <mhiramat@kernel.org> wrote:
> > > > >
> > > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > >
> > > > > This allows fprobes to be available with CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > > > > instead of CONFIG_DYNAMIC_FTRACE_WITH_REGS, then we can enable fprobe
> > > > > on arm64.
> > > >
> > > > This patch lets fprobe code build on configs WITH_ARGS and !WITH_REGS
> > > > but fprobe wouldn't run on these builds because fprobe still registers
> > > > to ftrace with FTRACE_OPS_FL_SAVE_REGS, which would fail on
> > > > !WITH_REGS. Shouldn't we also let the fprobe_init callers decide
> > > > whether they want REGS or not ?
> > >
> > > Ah, I think you meant FPROBE_EVENTS? Yes I forgot to add the dependency
> > > on it. But fprobe itself can work because fprobe just pass the ftrace_regs
> > > to the handlers. (Note that exit callback may not work until next patch)
> >
> > No, I mean that fprobe still registers its ftrace ops with the
> > FTRACE_OPS_FL_SAVE_REGS flag:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/kernel/trace/fprobe.c?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n185
> >
> > Which means that __register_ftrace_function will return -EINVAL on
> > builds !WITH_REGS:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/kernel/trace/ftrace.c?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n338
> >
> > As documented here:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/tree/include/linux/ftrace.h?h=topic/fprobe-ftrace-regs&id=2ca022b2753ae0d2a2513c95f7ed5b5b727fb2c4#n188
> >
> > There are two parts to using sparse pt_regs. One is "static": having
> > WITH_ARGS in the config, the second one is "dynamic": a ftrace ops
> > needs to specify that it doesn't want to go through the ftrace
> > trampoline that saves a full pt_regs, by not giving
> > FTRACE_OPS_FL_SAVE_REGS. If we want fprobe to work on builds
> > !WITH_REGS then we should both remove Kconfig dependencies to
> > WITH_REGS (like you've done) but also stop passing this ftrace ops
> > flag.
> 
> Said in a different way: there are arches that support both WITH_ARGS
> and WITH_REGS (like x86 actually). They have two ftrace trampolines
> compiled in: ftrace_caller and ftrace_regs_caller, one for each
> usecase. If you register to ftrace with the FTRACE_OPS_FL_SAVE_REGS
> flag you are telling it that what you want is a pt_regs. If you are
> trying to move away from pt_regs and support ftrace_regs in the more
> general case (meaning, in the case where it can contain a sparse
> pt_regs) then you should stop passing that flag so you go through the
> lighter, faster trampoline and test your code in the circumstances
> where ftrace_regs isn't just a regular pt_regs but an actually sparse
> or light data structure.
> 
> I hope that makes my thoughts clearer? It's a hairy topic ahah

Ah, I see your point.

static void fprobe_init(struct fprobe *fp)
{
        fp->nmissed = 0;
        if (fprobe_shared_with_kprobes(fp))
                fp->ops.func = fprobe_kprobe_handler;
        else
                fp->ops.func = fprobe_handler;
        fp->ops.flags |= FTRACE_OPS_FL_SAVE_REGS; <---- This flag!
}

So it should be FTRACE_OPS_FL_ARGS. Let me fix that.

Thank you!
-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [RFC PATCH v2 1/6] fprobe: Use fprobe_regs in fprobe entry handler

[Steven Rostedt]

On Thu, 10 Aug 2023 07:13:30 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > 
> > I hope that makes my thoughts clearer? It's a hairy topic ahah  
> 
> Ah, I see your point.
> 
> static void fprobe_init(struct fprobe *fp)
> {
>         fp->nmissed = 0;
>         if (fprobe_shared_with_kprobes(fp))
>                 fp->ops.func = fprobe_kprobe_handler;
>         else
>                 fp->ops.func = fprobe_handler;
>         fp->ops.flags |= FTRACE_OPS_FL_SAVE_REGS; <---- This flag!
> }
> 
> So it should be FTRACE_OPS_FL_ARGS. Let me fix that.

Yes, this was the concern that I was bringing up, where I did not see an
advantage of fprobes over kprobes using ftrace, because they both were
saving all registers.

-- Steve

[RFC PATCH v2 2/6] tracing: Expose ftrace_regs regardless of CONFIG_FUNCTION_TRACER

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In order to be able to use ftrace_regs even from features unrelated to
function tracer (e.g. kretprobe), expose ftrace_regs structures and
APIs even if the CONFIG_FUNCTION_TRACER=n.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/ftrace.h |   45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index ce156c7704ee..3fb94a1a2461 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -112,11 +112,11 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val
 }
 #endif
 
-#ifdef CONFIG_FUNCTION_TRACER
-
-extern int ftrace_enabled;
-
-#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+/*
+ * If the architecture doesn't support FTRACE_WITH_ARGS or disable function
+ * tracer, define the default(pt_regs compatible) ftrace_regs.
+ */
+#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || !defined(CONFIG_FUNCTION_TRACER)
 
 struct ftrace_regs {
 	struct pt_regs		regs;
@@ -129,6 +129,22 @@ struct ftrace_regs {
  * HAVE_DYNAMIC_FTRACE_WITH_ARGS is set and it supports live kernel patching.
  */
 #define ftrace_regs_set_instruction_pointer(fregs, ip) do { } while (0)
+
+#define ftrace_regs_get_instruction_pointer(fregs) \
+	instruction_pointer(ftrace_get_regs(fregs))
+#define ftrace_regs_get_argument(fregs, n) \
+	regs_get_kernel_argument(ftrace_get_regs(fregs), n)
+#define ftrace_regs_get_stack_pointer(fregs) \
+	kernel_stack_pointer(ftrace_get_regs(fregs))
+#define ftrace_regs_return_value(fregs) \
+	regs_return_value(ftrace_get_regs(fregs))
+#define ftrace_regs_set_return_value(fregs, ret) \
+	regs_set_return_value(ftrace_get_regs(fregs), ret)
+#define ftrace_override_function_with_return(fregs) \
+	override_function_with_return(ftrace_get_regs(fregs))
+#define ftrace_regs_query_register_offset(name) \
+	regs_query_register_offset(name)
+
 #endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
 
 static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs)
@@ -151,22 +167,9 @@ static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs)
 	return ftrace_get_regs(fregs) != NULL;
 }
 
-#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
-#define ftrace_regs_get_instruction_pointer(fregs) \
-	instruction_pointer(ftrace_get_regs(fregs))
-#define ftrace_regs_get_argument(fregs, n) \
-	regs_get_kernel_argument(ftrace_get_regs(fregs), n)
-#define ftrace_regs_get_stack_pointer(fregs) \
-	kernel_stack_pointer(ftrace_get_regs(fregs))
-#define ftrace_regs_return_value(fregs) \
-	regs_return_value(ftrace_get_regs(fregs))
-#define ftrace_regs_set_return_value(fregs, ret) \
-	regs_set_return_value(ftrace_get_regs(fregs), ret)
-#define ftrace_override_function_with_return(fregs) \
-	override_function_with_return(ftrace_get_regs(fregs))
-#define ftrace_regs_query_register_offset(name) \
-	regs_query_register_offset(name)
-#endif
+#ifdef CONFIG_FUNCTION_TRACER
+
+extern int ftrace_enabled;
 
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
 			      struct ftrace_ops *op, struct ftrace_regs *fregs);

Re: [RFC PATCH v2 2/6] tracing: Expose ftrace_regs regardless of CONFIG_FUNCTION_TRACER

[Florent Revest]

On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index ce156c7704ee..3fb94a1a2461 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -112,11 +112,11 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val
>  }
>  #endif
>
> -#ifdef CONFIG_FUNCTION_TRACER
> -
> -extern int ftrace_enabled;
> -
> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +/*
> + * If the architecture doesn't support FTRACE_WITH_ARGS or disable function

nit: disables*

> + * tracer, define the default(pt_regs compatible) ftrace_regs.
> + */
> +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || !defined(CONFIG_FUNCTION_TRACER)

I wonder if we should make things simpler with:

#if defined(HAVE_PT_REGS_COMPAT_FTRACE_REGS) || !defined(CONFIG_FUNCTION_TRACER)

And remove the ftrace_regs definitions that are copy-pastes of this
block in arch specific headers. Then we can enforce in a single point
that HAVE_PT_REGS_COMPAT_FTRACE_REGS holds.

Maybe that's a question for Steven ?

Re: [RFC PATCH v2 2/6] tracing: Expose ftrace_regs regardless of CONFIG_FUNCTION_TRACER

[Masami Hiramatsu]

On Wed, 9 Aug 2023 12:29:13 +0200
Florent Revest <revest@chromium.org> wrote:

> On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index ce156c7704ee..3fb94a1a2461 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -112,11 +112,11 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val
> >  }
> >  #endif
> >
> > -#ifdef CONFIG_FUNCTION_TRACER
> > -
> > -extern int ftrace_enabled;
> > -
> > -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > +/*
> > + * If the architecture doesn't support FTRACE_WITH_ARGS or disable function
> 
> nit: disables*

Thanks!

> 
> > + * tracer, define the default(pt_regs compatible) ftrace_regs.
> > + */
> > +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || !defined(CONFIG_FUNCTION_TRACER)
> 
> I wonder if we should make things simpler with:
> 
> #if defined(HAVE_PT_REGS_COMPAT_FTRACE_REGS) || !defined(CONFIG_FUNCTION_TRACER)
> 
> And remove the ftrace_regs definitions that are copy-pastes of this
> block in arch specific headers. Then we can enforce in a single point
> that HAVE_PT_REGS_COMPAT_FTRACE_REGS holds.

Here, the "HAVE_PT_REGS_COMPAT_FTRACE_REGS" does not mean that the
ftrace_regs is completely compatible with pt_regs, but on the memory
it wraps struct pt_regs (thus we can just cast the type).

- CONFIG_DYNAMIC_FTRACE_WITH_REGS
  ftrace_regs is completely compatible with pt_regs

- CONFIG_DYNAMIC_FTRACE_WITH_ARGS
  |   ftrace_regs may not compatible with pt_regs
  |
  +- CONFIG_HAVE_PT_REGS_COMPAT_FTRACE_REGS
     but on memory image, ftrace_regs includes pt_regs.

Thank you,

> 
> Maybe that's a question for Steven ?


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

Re: [RFC PATCH v2 2/6] tracing: Expose ftrace_regs regardless of CONFIG_FUNCTION_TRACER

[Florent Revest]

On Wed, Aug 9, 2023 at 4:16 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Wed, 9 Aug 2023 12:29:13 +0200
> Florent Revest <revest@chromium.org> wrote:
> > > + * tracer, define the default(pt_regs compatible) ftrace_regs.
> > > + */
> > > +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || !defined(CONFIG_FUNCTION_TRACER)
> >
> > I wonder if we should make things simpler with:
> >
> > #if defined(HAVE_PT_REGS_COMPAT_FTRACE_REGS) || !defined(CONFIG_FUNCTION_TRACER)
> >
> > And remove the ftrace_regs definitions that are copy-pastes of this
> > block in arch specific headers. Then we can enforce in a single point
> > that HAVE_PT_REGS_COMPAT_FTRACE_REGS holds.
>
> Here, the "HAVE_PT_REGS_COMPAT_FTRACE_REGS" does not mean that the
> ftrace_regs is completely compatible with pt_regs, but on the memory
> it wraps struct pt_regs (thus we can just cast the type).

But in practice I think that all architectures that chose to wrap a
pt_regs in their ftrace_regs also do:

+#define ftrace_regs_get_instruction_pointer(fregs) \
+       instruction_pointer(ftrace_get_regs(fregs))
+#define ftrace_regs_get_argument(fregs, n) \
+       regs_get_kernel_argument(ftrace_get_regs(fregs), n)
+#define ftrace_regs_get_stack_pointer(fregs) \
+       kernel_stack_pointer(ftrace_get_regs(fregs))
+#define ftrace_regs_return_value(fregs) \
+       regs_return_value(ftrace_get_regs(fregs))
+#define ftrace_regs_set_return_value(fregs, ret) \
+       regs_set_return_value(ftrace_get_regs(fregs), ret)
+#define ftrace_override_function_with_return(fregs) \
+       override_function_with_return(ftrace_get_regs(fregs))
+#define ftrace_regs_query_register_offset(name) \
+       regs_query_register_offset(name)

And are just careful to populate the fields that let these macros
work. So maybe these could be factorized... But anyway, I'm not
particularly a super fan of the idea and I don't think it should
necessarily fit in that series. It's just something that crossed my
mind, if you're not a fan then we should probably not do it ;)

[RFC PATCH v2 3/6] fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Change the fprobe exit handler and rethook to use ftrace_regs data structure
instead of pt_regs. This also introduce HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS
which means the ftrace_regs is equal to the pt_regs so that those are
compatible. Only if it is enabled, kretprobe will use rethook since kretprobe
requires pt_regs for backward compatibility.

This means the archs which currently implement rethook for kretprobes needs to
set that flag and it must ensure struct ftrace_regs is same as pt_regs.
If not, it must be either disabling kretprobe or implementing kretprobe
trampoline separately from rethook trampoline.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 arch/Kconfig                    |    1 +
 arch/loongarch/Kconfig          |    1 +
 arch/s390/Kconfig               |    1 +
 arch/x86/Kconfig                |    1 +
 arch/x86/kernel/rethook.c       |    9 ++++++---
 include/linux/fprobe.h          |    2 +-
 include/linux/rethook.h         |   11 ++++++-----
 kernel/kprobes.c                |    9 +++++++--
 kernel/trace/Kconfig            |    7 +++++++
 kernel/trace/bpf_trace.c        |    6 +++++-
 kernel/trace/fprobe.c           |    6 +++---
 kernel/trace/rethook.c          |   16 ++++++++--------
 kernel/trace/trace_fprobe.c     |    6 +++++-
 lib/test_fprobe.c               |    6 +++---
 samples/fprobe/fprobe_example.c |    2 +-
 15 files changed, 56 insertions(+), 28 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index aff2746c8af2..e321bdb8b22b 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -201,6 +201,7 @@ config KRETPROBE_ON_RETHOOK
 	def_bool y
 	depends on HAVE_RETHOOK
 	depends on KRETPROBES
+	depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS || !HAVE_DYNAMIC_FTRACE_WITH_ARGS
 	select RETHOOK
 
 config USER_RETURN_NOTIFIER
diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index e55511af4c77..93a4336b0a94 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -102,6 +102,7 @@ config LOONGARCH
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_ARGS
+	select HAVE_PT_REGS_COMPAT_FTRACE_REGS
 	select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_EBPF_JIT
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 5b39918b7042..299ba17d3316 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -165,6 +165,7 @@ config S390
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_ARGS
+	select HAVE_PT_REGS_COMPAT_FTRACE_REGS
 	select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_EBPF_JIT if HAVE_MARCH_Z196_FEATURES
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 7422db409770..df1b7a2791e8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -207,6 +207,7 @@ config X86
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if X86_64
+	select HAVE_PT_REGS_COMPAT_FTRACE_REGS	if X86_64
 	select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	select HAVE_SAMPLE_FTRACE_DIRECT	if X86_64
 	select HAVE_SAMPLE_FTRACE_DIRECT_MULTI	if X86_64
diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
index 8a1c0111ae79..79a52bfde562 100644
--- a/arch/x86/kernel/rethook.c
+++ b/arch/x86/kernel/rethook.c
@@ -83,7 +83,8 @@ __used __visible void arch_rethook_trampoline_callback(struct pt_regs *regs)
 	 * arch_rethook_fixup_return() which called from this
 	 * rethook_trampoline_handler().
 	 */
-	rethook_trampoline_handler(regs, (unsigned long)frame_pointer);
+	rethook_trampoline_handler((struct ftrace_regs *)regs,
+				   (unsigned long)frame_pointer);
 
 	/*
 	 * Copy FLAGS to 'pt_regs::ss' so that arch_rethook_trapmoline()
@@ -104,9 +105,10 @@ NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
 STACK_FRAME_NON_STANDARD_FP(arch_rethook_trampoline);
 
 /* This is called from rethook_trampoline_handler(). */
-void arch_rethook_fixup_return(struct pt_regs *regs,
+void arch_rethook_fixup_return(struct ftrace_regs *fregs,
 			       unsigned long correct_ret_addr)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);
 	unsigned long *frame_pointer = (void *)(regs + 1);
 
 	/* Replace fake return address with real one. */
@@ -114,8 +116,9 @@ void arch_rethook_fixup_return(struct pt_regs *regs,
 }
 NOKPROBE_SYMBOL(arch_rethook_fixup_return);
 
-void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
+void arch_rethook_prepare(struct rethook_node *rh, struct ftrace_regs *fregs, bool mcount)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);
 	unsigned long *stack = (unsigned long *)regs->sp;
 
 	rh->ret_addr = stack[0];
diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
index 36c0595f7b93..b9c0c216dedb 100644
--- a/include/linux/fprobe.h
+++ b/include/linux/fprobe.h
@@ -38,7 +38,7 @@ struct fprobe {
 			     unsigned long ret_ip, struct ftrace_regs *regs,
 			     void *entry_data);
 	void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
-			     unsigned long ret_ip, struct pt_regs *regs,
+			     unsigned long ret_ip, struct ftrace_regs *regs,
 			     void *entry_data);
 };
 
diff --git a/include/linux/rethook.h b/include/linux/rethook.h
index 26b6f3c81a76..138d64c8b67b 100644
--- a/include/linux/rethook.h
+++ b/include/linux/rethook.h
@@ -7,6 +7,7 @@
 
 #include <linux/compiler.h>
 #include <linux/freelist.h>
+#include <linux/ftrace.h>
 #include <linux/kallsyms.h>
 #include <linux/llist.h>
 #include <linux/rcupdate.h>
@@ -14,7 +15,7 @@
 
 struct rethook_node;
 
-typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct pt_regs *);
+typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct ftrace_regs *);
 
 /**
  * struct rethook - The rethook management data structure.
@@ -64,12 +65,12 @@ void rethook_free(struct rethook *rh);
 void rethook_add_node(struct rethook *rh, struct rethook_node *node);
 struct rethook_node *rethook_try_get(struct rethook *rh);
 void rethook_recycle(struct rethook_node *node);
-void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount);
+void rethook_hook(struct rethook_node *node, struct ftrace_regs *regs, bool mcount);
 unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame,
 				    struct llist_node **cur);
 
 /* Arch dependent code must implement arch_* and trampoline code */
-void arch_rethook_prepare(struct rethook_node *node, struct pt_regs *regs, bool mcount);
+void arch_rethook_prepare(struct rethook_node *node, struct ftrace_regs *regs, bool mcount);
 void arch_rethook_trampoline(void);
 
 /**
@@ -84,11 +85,11 @@ static inline bool is_rethook_trampoline(unsigned long addr)
 }
 
 /* If the architecture needs to fixup the return address, implement it. */
-void arch_rethook_fixup_return(struct pt_regs *regs,
+void arch_rethook_fixup_return(struct ftrace_regs *regs,
 			       unsigned long correct_ret_addr);
 
 /* Generic trampoline handler, arch code must prepare asm stub */
-unsigned long rethook_trampoline_handler(struct pt_regs *regs,
+unsigned long rethook_trampoline_handler(struct ftrace_regs *regs,
 					 unsigned long frame);
 
 #ifdef CONFIG_RETHOOK
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 1fc6095d502d..ccbe41c961c3 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2120,7 +2120,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
 	if (rp->entry_handler && rp->entry_handler(ri, regs))
 		rethook_recycle(rhn);
 	else
-		rethook_hook(rhn, regs, kprobe_ftrace(p));
+		rethook_hook(rhn, (struct ftrace_regs *)regs, kprobe_ftrace(p));
 
 	return 0;
 }
@@ -2128,12 +2128,17 @@ NOKPROBE_SYMBOL(pre_handler_kretprobe);
 
 static void kretprobe_rethook_handler(struct rethook_node *rh, void *data,
 				      unsigned long ret_addr,
-				      struct pt_regs *regs)
+				      struct ftrace_regs *fregs)
 {
 	struct kretprobe *rp = (struct kretprobe *)data;
+	struct pt_regs *regs = ftrace_get_regs(fregs);
 	struct kretprobe_instance *ri;
 	struct kprobe_ctlblk *kcb;
 
+	/* This is bug because this depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS */
+	if (WARN_ON_ONCE(!regs))
+		return;
+
 	/* The data must NOT be null. This means rethook data structure is broken. */
 	if (WARN_ON_ONCE(!data) || !rp->handler)
 		return;
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 976fd594b446..7d6abb5bd861 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -57,6 +57,13 @@ config HAVE_DYNAMIC_FTRACE_WITH_ARGS
 	 This allows for use of ftrace_regs_get_argument() and
 	 ftrace_regs_get_stack_pointer().
 
+config HAVE_PT_REGS_COMPAT_FTRACE_REGS
+	bool
+	help
+	 If this is set, the ftrace_regs data structure is compatible
+	 with the pt_regs. So it is possible to be converted by
+	 ftrace_get_regs().
+
 config HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
 	bool
 	help
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 51573eaa04c4..99c5f95360f9 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2668,10 +2668,14 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
 
 static void
 kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
-			       unsigned long ret_ip, struct pt_regs *regs,
+			       unsigned long ret_ip, struct ftrace_regs *fregs,
 			       void *data)
 {
 	struct bpf_kprobe_multi_link *link;
+	struct pt_regs *regs = ftrace_get_regs(fregs);
+
+	if (!regs)
+		return;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
 	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index 15a2aef92733..70b9c493e52d 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -53,7 +53,7 @@ static inline void __fprobe_handler(unsigned long ip, unsigned long parent_ip,
 		if (ret)
 			rethook_recycle(rh);
 		else
-			rethook_hook(rh, ftrace_get_regs(fregs), true);
+			rethook_hook(rh, fregs, true);
 	}
 }
 
@@ -120,7 +120,7 @@ static void fprobe_kprobe_handler(unsigned long ip, unsigned long parent_ip,
 }
 
 static void fprobe_exit_handler(struct rethook_node *rh, void *data,
-				unsigned long ret_ip, struct pt_regs *regs)
+				unsigned long ret_ip, struct ftrace_regs *fregs)
 {
 	struct fprobe *fp = (struct fprobe *)data;
 	struct fprobe_rethook_node *fpr;
@@ -141,7 +141,7 @@ static void fprobe_exit_handler(struct rethook_node *rh, void *data,
 		return;
 	}
 
-	fp->exit_handler(fp, fpr->entry_ip, ret_ip, regs,
+	fp->exit_handler(fp, fpr->entry_ip, ret_ip, fregs,
 			 fp->entry_data_size ? (void *)fpr->data : NULL);
 	ftrace_test_recursion_unlock(bit);
 }
diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
index 5eb9b598f4e9..7c5cf9d5910c 100644
--- a/kernel/trace/rethook.c
+++ b/kernel/trace/rethook.c
@@ -189,7 +189,7 @@ NOKPROBE_SYMBOL(rethook_try_get);
 /**
  * rethook_hook() - Hook the current function return.
  * @node: The struct rethook node to hook the function return.
- * @regs: The struct pt_regs for the function entry.
+ * @fregs: The struct ftrace_regs for the function entry.
  * @mcount: True if this is called from mcount(ftrace) context.
  *
  * Hook the current running function return. This must be called when the
@@ -199,9 +199,9 @@ NOKPROBE_SYMBOL(rethook_try_get);
  * from the real function entry (e.g. kprobes) @mcount must be set false.
  * This is because the way to hook the function return depends on the context.
  */
-void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount)
+void rethook_hook(struct rethook_node *node, struct ftrace_regs *fregs, bool mcount)
 {
-	arch_rethook_prepare(node, regs, mcount);
+	arch_rethook_prepare(node, fregs, mcount);
 	__llist_add(&node->llist, &current->rethooks);
 }
 NOKPROBE_SYMBOL(rethook_hook);
@@ -269,7 +269,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
 }
 NOKPROBE_SYMBOL(rethook_find_ret_addr);
 
-void __weak arch_rethook_fixup_return(struct pt_regs *regs,
+void __weak arch_rethook_fixup_return(struct ftrace_regs *fregs,
 				      unsigned long correct_ret_addr)
 {
 	/*
@@ -281,7 +281,7 @@ void __weak arch_rethook_fixup_return(struct pt_regs *regs,
 }
 
 /* This function will be called from each arch-defined trampoline. */
-unsigned long rethook_trampoline_handler(struct pt_regs *regs,
+unsigned long rethook_trampoline_handler(struct ftrace_regs *fregs,
 					 unsigned long frame)
 {
 	struct llist_node *first, *node = NULL;
@@ -295,7 +295,7 @@ unsigned long rethook_trampoline_handler(struct pt_regs *regs,
 		BUG_ON(1);
 	}
 
-	instruction_pointer_set(regs, correct_ret_addr);
+	ftrace_regs_set_instruction_pointer(fregs, correct_ret_addr);
 
 	/*
 	 * These loops must be protected from rethook_free_rcu() because those
@@ -315,7 +315,7 @@ unsigned long rethook_trampoline_handler(struct pt_regs *regs,
 		handler = READ_ONCE(rhn->rethook->handler);
 		if (handler)
 			handler(rhn, rhn->rethook->data,
-				correct_ret_addr, regs);
+				correct_ret_addr, fregs);
 
 		if (first == node)
 			break;
@@ -323,7 +323,7 @@ unsigned long rethook_trampoline_handler(struct pt_regs *regs,
 	}
 
 	/* Fixup registers for returning to correct address. */
-	arch_rethook_fixup_return(regs, correct_ret_addr);
+	arch_rethook_fixup_return(fregs, correct_ret_addr);
 
 	/* Unlink used shadow stack */
 	first = current->rethooks.first;
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 4d3ae79f036e..f440c97e050f 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -341,10 +341,14 @@ static int fentry_dispatcher(struct fprobe *fp, unsigned long entry_ip,
 NOKPROBE_SYMBOL(fentry_dispatcher);
 
 static void fexit_dispatcher(struct fprobe *fp, unsigned long entry_ip,
-			     unsigned long ret_ip, struct pt_regs *regs,
+			     unsigned long ret_ip, struct ftrace_regs *fregs,
 			     void *entry_data)
 {
 	struct trace_fprobe *tf = container_of(fp, struct trace_fprobe, fp);
+	struct pt_regs *regs = ftrace_get_regs(fregs);
+
+	if (!regs)
+		return;
 
 	if (trace_probe_test_flag(&tf->tp, TP_FLAG_TRACE))
 		fexit_trace_func(tf, entry_ip, ret_ip, regs);
diff --git a/lib/test_fprobe.c b/lib/test_fprobe.c
index ff607babba18..d1e80653bf0c 100644
--- a/lib/test_fprobe.c
+++ b/lib/test_fprobe.c
@@ -59,9 +59,9 @@ static notrace int fp_entry_handler(struct fprobe *fp, unsigned long ip,
 
 static notrace void fp_exit_handler(struct fprobe *fp, unsigned long ip,
 				    unsigned long ret_ip,
-				    struct pt_regs *regs, void *data)
+				    struct ftrace_regs *fregs, void *data)
 {
-	unsigned long ret = regs_return_value(regs);
+	unsigned long ret = ftrace_regs_return_value(fregs);
 
 	KUNIT_EXPECT_FALSE(current_test, preemptible());
 	if (ip != target_ip) {
@@ -89,7 +89,7 @@ static notrace int nest_entry_handler(struct fprobe *fp, unsigned long ip,
 
 static notrace void nest_exit_handler(struct fprobe *fp, unsigned long ip,
 				      unsigned long ret_ip,
-				      struct pt_regs *regs, void *data)
+				      struct ftrace_regs *fregs, void *data)
 {
 	KUNIT_EXPECT_FALSE(current_test, preemptible());
 	KUNIT_EXPECT_EQ(current_test, ip, target_nest_ip);
diff --git a/samples/fprobe/fprobe_example.c b/samples/fprobe/fprobe_example.c
index 1545a1aac616..d476d1f07538 100644
--- a/samples/fprobe/fprobe_example.c
+++ b/samples/fprobe/fprobe_example.c
@@ -67,7 +67,7 @@ static int sample_entry_handler(struct fprobe *fp, unsigned long ip,
 }
 
 static void sample_exit_handler(struct fprobe *fp, unsigned long ip,
-				unsigned long ret_ip, struct pt_regs *regs,
+				unsigned long ret_ip, struct ftrace_regs *regs,
 				void *data)
 {
 	unsigned long rip = ret_ip;

Re: [RFC PATCH v2 3/6] fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook

[Florent Revest]

The title says "Use fprobe_regs", I think you meant ftrace_regs (we
have enough problems with two regs structs already! :o) ) same comment
for patch 1

On Mon, Aug 7, 2023 at 8:49 AM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Change the fprobe exit handler and rethook to use ftrace_regs data structure
> instead of pt_regs. This also introduce HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS

The macro name in the patch description doesn't match the one in the
patch (HAVE_PT_REGS_COMPAT_FTRACE_REGS)

> which means the ftrace_regs is equal to the pt_regs so that those are
> compatible. Only if it is enabled, kretprobe will use rethook since kretprobe
> requires pt_regs for backward compatibility.
>
> This means the archs which currently implement rethook for kretprobes needs to
> set that flag and it must ensure struct ftrace_regs is same as pt_regs.

nit: I'm a bit confused when you say that these structures are "the
same". Let's take x86 as an example, ftrace_regs is "the same" as
pt_regs in the sense that they use the same space in memory and have
the same fields alignment but my understanding of ftrace_regs is that
it can either be a full pt_regs or a sparse pt_regs. If the trampoline
doesn't go through ftrace_regs_caller_op_ptr, registers like CS will
be missing and ftrace_get_regs will return NULL, I don't call it being
"the same". I guess what you mean here is that a pt_regs can be casted
back into a ftrace_regs. Maybe HAVE_PT_REGS_TO_FTRACE_REGS_CAST would
be a clearer name ?

> If not, it must be either disabling kretprobe or implementing kretprobe
> trampoline separately from rethook trampoline.
>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  arch/Kconfig                    |    1 +
>  arch/loongarch/Kconfig          |    1 +
>  arch/s390/Kconfig               |    1 +
>  arch/x86/Kconfig                |    1 +
>  arch/x86/kernel/rethook.c       |    9 ++++++---
>  include/linux/fprobe.h          |    2 +-
>  include/linux/rethook.h         |   11 ++++++-----
>  kernel/kprobes.c                |    9 +++++++--
>  kernel/trace/Kconfig            |    7 +++++++
>  kernel/trace/bpf_trace.c        |    6 +++++-
>  kernel/trace/fprobe.c           |    6 +++---
>  kernel/trace/rethook.c          |   16 ++++++++--------
>  kernel/trace/trace_fprobe.c     |    6 +++++-
>  lib/test_fprobe.c               |    6 +++---
>  samples/fprobe/fprobe_example.c |    2 +-
>  15 files changed, 56 insertions(+), 28 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index aff2746c8af2..e321bdb8b22b 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -201,6 +201,7 @@ config KRETPROBE_ON_RETHOOK
>         def_bool y
>         depends on HAVE_RETHOOK
>         depends on KRETPROBES
> +       depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS || !HAVE_DYNAMIC_FTRACE_WITH_ARGS
>         select RETHOOK
>
>  config USER_RETURN_NOTIFIER
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index e55511af4c77..93a4336b0a94 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -102,6 +102,7 @@ config LOONGARCH
>         select HAVE_DMA_CONTIGUOUS
>         select HAVE_DYNAMIC_FTRACE
>         select HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +       select HAVE_PT_REGS_COMPAT_FTRACE_REGS
>         select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>         select HAVE_DYNAMIC_FTRACE_WITH_REGS
>         select HAVE_EBPF_JIT
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 5b39918b7042..299ba17d3316 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -165,6 +165,7 @@ config S390
>         select HAVE_DMA_CONTIGUOUS
>         select HAVE_DYNAMIC_FTRACE
>         select HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +       select HAVE_PT_REGS_COMPAT_FTRACE_REGS
>         select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>         select HAVE_DYNAMIC_FTRACE_WITH_REGS
>         select HAVE_EBPF_JIT if HAVE_MARCH_Z196_FEATURES
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 7422db409770..df1b7a2791e8 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -207,6 +207,7 @@ config X86
>         select HAVE_DYNAMIC_FTRACE
>         select HAVE_DYNAMIC_FTRACE_WITH_REGS
>         select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if X86_64
> +       select HAVE_PT_REGS_COMPAT_FTRACE_REGS  if X86_64
>         select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>         select HAVE_SAMPLE_FTRACE_DIRECT        if X86_64
>         select HAVE_SAMPLE_FTRACE_DIRECT_MULTI  if X86_64
> diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
> index 8a1c0111ae79..79a52bfde562 100644
> --- a/arch/x86/kernel/rethook.c
> +++ b/arch/x86/kernel/rethook.c
> @@ -83,7 +83,8 @@ __used __visible void arch_rethook_trampoline_callback(struct pt_regs *regs)
>          * arch_rethook_fixup_return() which called from this
>          * rethook_trampoline_handler().
>          */
> -       rethook_trampoline_handler(regs, (unsigned long)frame_pointer);
> +       rethook_trampoline_handler((struct ftrace_regs *)regs,
> +                                  (unsigned long)frame_pointer);
>
>         /*
>          * Copy FLAGS to 'pt_regs::ss' so that arch_rethook_trapmoline()
> @@ -104,9 +105,10 @@ NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
>  STACK_FRAME_NON_STANDARD_FP(arch_rethook_trampoline);
>
>  /* This is called from rethook_trampoline_handler(). */
> -void arch_rethook_fixup_return(struct pt_regs *regs,
> +void arch_rethook_fixup_return(struct ftrace_regs *fregs,
>                                unsigned long correct_ret_addr)
>  {
> +       struct pt_regs *regs = ftrace_get_regs(fregs);
>         unsigned long *frame_pointer = (void *)(regs + 1);
>
>         /* Replace fake return address with real one. */
> @@ -114,8 +116,9 @@ void arch_rethook_fixup_return(struct pt_regs *regs,
>  }
>  NOKPROBE_SYMBOL(arch_rethook_fixup_return);
>
> -void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> +void arch_rethook_prepare(struct rethook_node *rh, struct ftrace_regs *fregs, bool mcount)
>  {
> +       struct pt_regs *regs = ftrace_get_regs(fregs);
>         unsigned long *stack = (unsigned long *)regs->sp;
>
>         rh->ret_addr = stack[0];
> diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> index 36c0595f7b93..b9c0c216dedb 100644
> --- a/include/linux/fprobe.h
> +++ b/include/linux/fprobe.h
> @@ -38,7 +38,7 @@ struct fprobe {
>                              unsigned long ret_ip, struct ftrace_regs *regs,
>                              void *entry_data);
>         void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
> -                            unsigned long ret_ip, struct pt_regs *regs,
> +                            unsigned long ret_ip, struct ftrace_regs *regs,
>                              void *entry_data);
>  };
>
> diff --git a/include/linux/rethook.h b/include/linux/rethook.h
> index 26b6f3c81a76..138d64c8b67b 100644
> --- a/include/linux/rethook.h
> +++ b/include/linux/rethook.h
> @@ -7,6 +7,7 @@
>
>  #include <linux/compiler.h>
>  #include <linux/freelist.h>
> +#include <linux/ftrace.h>
>  #include <linux/kallsyms.h>
>  #include <linux/llist.h>
>  #include <linux/rcupdate.h>
> @@ -14,7 +15,7 @@
>
>  struct rethook_node;
>
> -typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct pt_regs *);
> +typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct ftrace_regs *);
>
>  /**
>   * struct rethook - The rethook management data structure.
> @@ -64,12 +65,12 @@ void rethook_free(struct rethook *rh);
>  void rethook_add_node(struct rethook *rh, struct rethook_node *node);
>  struct rethook_node *rethook_try_get(struct rethook *rh);
>  void rethook_recycle(struct rethook_node *node);
> -void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount);
> +void rethook_hook(struct rethook_node *node, struct ftrace_regs *regs, bool mcount);
>  unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame,
>                                     struct llist_node **cur);
>
>  /* Arch dependent code must implement arch_* and trampoline code */
> -void arch_rethook_prepare(struct rethook_node *node, struct pt_regs *regs, bool mcount);
> +void arch_rethook_prepare(struct rethook_node *node, struct ftrace_regs *regs, bool mcount);
>  void arch_rethook_trampoline(void);
>
>  /**
> @@ -84,11 +85,11 @@ static inline bool is_rethook_trampoline(unsigned long addr)
>  }
>
>  /* If the architecture needs to fixup the return address, implement it. */
> -void arch_rethook_fixup_return(struct pt_regs *regs,
> +void arch_rethook_fixup_return(struct ftrace_regs *regs,
>                                unsigned long correct_ret_addr);
>
>  /* Generic trampoline handler, arch code must prepare asm stub */
> -unsigned long rethook_trampoline_handler(struct pt_regs *regs,
> +unsigned long rethook_trampoline_handler(struct ftrace_regs *regs,
>                                          unsigned long frame);
>
>  #ifdef CONFIG_RETHOOK
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 1fc6095d502d..ccbe41c961c3 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2120,7 +2120,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
>         if (rp->entry_handler && rp->entry_handler(ri, regs))
>                 rethook_recycle(rhn);
>         else
> -               rethook_hook(rhn, regs, kprobe_ftrace(p));
> +               rethook_hook(rhn, (struct ftrace_regs *)regs, kprobe_ftrace(p));

I think there are two things that can be meant with "rethook uses ftrace_regs":

- rethook callbacks receive a ftrace_regs (that's what you do further down)
- rethook can hook to a traced function using a ftrace_regs (that's
what you use in fprobe now)

But I think the second proposition shouldn't imply that rethook_hook
can _only_ hook to ftrace_regs. For the kprobe use case, I think there
should also be a rethook_hook_pt_regs() that operates on a pt_regs. We
could have a default implementation of rethook_hook that calls into
the other (or vice versa) on HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS
but I think it's good to separate these two APIs

Re: [RFC PATCH v2 3/6] fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook

[Masami Hiramatsu]

On Wed, 9 Aug 2023 12:30:47 +0200
Florent Revest <revest@chromium.org> wrote:

> The title says "Use fprobe_regs", I think you meant ftrace_regs (we
> have enough problems with two regs structs already! :o) ) same comment
> for patch 1

Oops! thanks!

> 
> On Mon, Aug 7, 2023 at 8:49 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Change the fprobe exit handler and rethook to use ftrace_regs data structure
> > instead of pt_regs. This also introduce HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS
> 
> The macro name in the patch description doesn't match the one in the
> patch (HAVE_PT_REGS_COMPAT_FTRACE_REGS)

Ah, sorry, that is also a typo, it was the old macro name.
(but since it too long, I changed that)

> 
> > which means the ftrace_regs is equal to the pt_regs so that those are
> > compatible. Only if it is enabled, kretprobe will use rethook since kretprobe
> > requires pt_regs for backward compatibility.
> >
> > This means the archs which currently implement rethook for kretprobes needs to
> > set that flag and it must ensure struct ftrace_regs is same as pt_regs.
> 
> nit: I'm a bit confused when you say that these structures are "the
> same". Let's take x86 as an example, ftrace_regs is "the same" as
> pt_regs in the sense that they use the same space in memory and have
> the same fields alignment but my understanding of ftrace_regs is that
> it can either be a full pt_regs or a sparse pt_regs.

Yes, you're right. it must be changed that it means ftrace_regs is
just a wrapper of pt_regs, so the memory layout is the same.

> If the trampoline
> doesn't go through ftrace_regs_caller_op_ptr, registers like CS will
> be missing and ftrace_get_regs will return NULL, I don't call it being
> "the same". I guess what you mean here is that a pt_regs can be casted
> back into a ftrace_regs. Maybe HAVE_PT_REGS_TO_FTRACE_REGS_CAST would
> be a clearer name ?

Ok, let me change the name.

> 
> > If not, it must be either disabling kretprobe or implementing kretprobe
> > trampoline separately from rethook trampoline.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  arch/Kconfig                    |    1 +
> >  arch/loongarch/Kconfig          |    1 +
> >  arch/s390/Kconfig               |    1 +
> >  arch/x86/Kconfig                |    1 +
> >  arch/x86/kernel/rethook.c       |    9 ++++++---
> >  include/linux/fprobe.h          |    2 +-
> >  include/linux/rethook.h         |   11 ++++++-----
> >  kernel/kprobes.c                |    9 +++++++--
> >  kernel/trace/Kconfig            |    7 +++++++
> >  kernel/trace/bpf_trace.c        |    6 +++++-
> >  kernel/trace/fprobe.c           |    6 +++---
> >  kernel/trace/rethook.c          |   16 ++++++++--------
> >  kernel/trace/trace_fprobe.c     |    6 +++++-
> >  lib/test_fprobe.c               |    6 +++---
> >  samples/fprobe/fprobe_example.c |    2 +-
> >  15 files changed, 56 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index aff2746c8af2..e321bdb8b22b 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -201,6 +201,7 @@ config KRETPROBE_ON_RETHOOK
> >         def_bool y
> >         depends on HAVE_RETHOOK
> >         depends on KRETPROBES
> > +       depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS || !HAVE_DYNAMIC_FTRACE_WITH_ARGS
> >         select RETHOOK
> >
> >  config USER_RETURN_NOTIFIER
> > diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> > index e55511af4c77..93a4336b0a94 100644
> > --- a/arch/loongarch/Kconfig
> > +++ b/arch/loongarch/Kconfig
> > @@ -102,6 +102,7 @@ config LOONGARCH
> >         select HAVE_DMA_CONTIGUOUS
> >         select HAVE_DYNAMIC_FTRACE
> >         select HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > +       select HAVE_PT_REGS_COMPAT_FTRACE_REGS
> >         select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> >         select HAVE_DYNAMIC_FTRACE_WITH_REGS
> >         select HAVE_EBPF_JIT
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index 5b39918b7042..299ba17d3316 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -165,6 +165,7 @@ config S390
> >         select HAVE_DMA_CONTIGUOUS
> >         select HAVE_DYNAMIC_FTRACE
> >         select HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > +       select HAVE_PT_REGS_COMPAT_FTRACE_REGS
> >         select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> >         select HAVE_DYNAMIC_FTRACE_WITH_REGS
> >         select HAVE_EBPF_JIT if HAVE_MARCH_Z196_FEATURES
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 7422db409770..df1b7a2791e8 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -207,6 +207,7 @@ config X86
> >         select HAVE_DYNAMIC_FTRACE
> >         select HAVE_DYNAMIC_FTRACE_WITH_REGS
> >         select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if X86_64
> > +       select HAVE_PT_REGS_COMPAT_FTRACE_REGS  if X86_64
> >         select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> >         select HAVE_SAMPLE_FTRACE_DIRECT        if X86_64
> >         select HAVE_SAMPLE_FTRACE_DIRECT_MULTI  if X86_64
> > diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
> > index 8a1c0111ae79..79a52bfde562 100644
> > --- a/arch/x86/kernel/rethook.c
> > +++ b/arch/x86/kernel/rethook.c
> > @@ -83,7 +83,8 @@ __used __visible void arch_rethook_trampoline_callback(struct pt_regs *regs)
> >          * arch_rethook_fixup_return() which called from this
> >          * rethook_trampoline_handler().
> >          */
> > -       rethook_trampoline_handler(regs, (unsigned long)frame_pointer);
> > +       rethook_trampoline_handler((struct ftrace_regs *)regs,
> > +                                  (unsigned long)frame_pointer);
> >
> >         /*
> >          * Copy FLAGS to 'pt_regs::ss' so that arch_rethook_trapmoline()
> > @@ -104,9 +105,10 @@ NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
> >  STACK_FRAME_NON_STANDARD_FP(arch_rethook_trampoline);
> >
> >  /* This is called from rethook_trampoline_handler(). */
> > -void arch_rethook_fixup_return(struct pt_regs *regs,
> > +void arch_rethook_fixup_return(struct ftrace_regs *fregs,
> >                                unsigned long correct_ret_addr)
> >  {
> > +       struct pt_regs *regs = ftrace_get_regs(fregs);
> >         unsigned long *frame_pointer = (void *)(regs + 1);
> >
> >         /* Replace fake return address with real one. */
> > @@ -114,8 +116,9 @@ void arch_rethook_fixup_return(struct pt_regs *regs,
> >  }
> >  NOKPROBE_SYMBOL(arch_rethook_fixup_return);
> >
> > -void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> > +void arch_rethook_prepare(struct rethook_node *rh, struct ftrace_regs *fregs, bool mcount)
> >  {
> > +       struct pt_regs *regs = ftrace_get_regs(fregs);
> >         unsigned long *stack = (unsigned long *)regs->sp;
> >
> >         rh->ret_addr = stack[0];
> > diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> > index 36c0595f7b93..b9c0c216dedb 100644
> > --- a/include/linux/fprobe.h
> > +++ b/include/linux/fprobe.h
> > @@ -38,7 +38,7 @@ struct fprobe {
> >                              unsigned long ret_ip, struct ftrace_regs *regs,
> >                              void *entry_data);
> >         void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
> > -                            unsigned long ret_ip, struct pt_regs *regs,
> > +                            unsigned long ret_ip, struct ftrace_regs *regs,
> >                              void *entry_data);
> >  };
> >
> > diff --git a/include/linux/rethook.h b/include/linux/rethook.h
> > index 26b6f3c81a76..138d64c8b67b 100644
> > --- a/include/linux/rethook.h
> > +++ b/include/linux/rethook.h
> > @@ -7,6 +7,7 @@
> >
> >  #include <linux/compiler.h>
> >  #include <linux/freelist.h>
> > +#include <linux/ftrace.h>
> >  #include <linux/kallsyms.h>
> >  #include <linux/llist.h>
> >  #include <linux/rcupdate.h>
> > @@ -14,7 +15,7 @@
> >
> >  struct rethook_node;
> >
> > -typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct pt_regs *);
> > +typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct ftrace_regs *);
> >
> >  /**
> >   * struct rethook - The rethook management data structure.
> > @@ -64,12 +65,12 @@ void rethook_free(struct rethook *rh);
> >  void rethook_add_node(struct rethook *rh, struct rethook_node *node);
> >  struct rethook_node *rethook_try_get(struct rethook *rh);
> >  void rethook_recycle(struct rethook_node *node);
> > -void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount);
> > +void rethook_hook(struct rethook_node *node, struct ftrace_regs *regs, bool mcount);
> >  unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame,
> >                                     struct llist_node **cur);
> >
> >  /* Arch dependent code must implement arch_* and trampoline code */
> > -void arch_rethook_prepare(struct rethook_node *node, struct pt_regs *regs, bool mcount);
> > +void arch_rethook_prepare(struct rethook_node *node, struct ftrace_regs *regs, bool mcount);
> >  void arch_rethook_trampoline(void);
> >
> >  /**
> > @@ -84,11 +85,11 @@ static inline bool is_rethook_trampoline(unsigned long addr)
> >  }
> >
> >  /* If the architecture needs to fixup the return address, implement it. */
> > -void arch_rethook_fixup_return(struct pt_regs *regs,
> > +void arch_rethook_fixup_return(struct ftrace_regs *regs,
> >                                unsigned long correct_ret_addr);
> >
> >  /* Generic trampoline handler, arch code must prepare asm stub */
> > -unsigned long rethook_trampoline_handler(struct pt_regs *regs,
> > +unsigned long rethook_trampoline_handler(struct ftrace_regs *regs,
> >                                          unsigned long frame);
> >
> >  #ifdef CONFIG_RETHOOK
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 1fc6095d502d..ccbe41c961c3 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2120,7 +2120,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> >         if (rp->entry_handler && rp->entry_handler(ri, regs))
> >                 rethook_recycle(rhn);
> >         else
> > -               rethook_hook(rhn, regs, kprobe_ftrace(p));
> > +               rethook_hook(rhn, (struct ftrace_regs *)regs, kprobe_ftrace(p));
> 
> I think there are two things that can be meant with "rethook uses ftrace_regs":
> 
> - rethook callbacks receive a ftrace_regs (that's what you do further down)
> - rethook can hook to a traced function using a ftrace_regs (that's
> what you use in fprobe now)
> 
> But I think the second proposition shouldn't imply that rethook_hook
> can _only_ hook to ftrace_regs. For the kprobe use case, I think there
> should also be a rethook_hook_pt_regs() that operates on a pt_regs. We
> could have a default implementation of rethook_hook that calls into
> the other (or vice versa) on HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS
> but I think it's good to separate these two APIs

Yeah, so for simplying the 2nd case, I added this dependency.

diff --git a/arch/Kconfig b/arch/Kconfig
index aff2746c8af2..e321bdb8b22b 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -201,6 +201,7 @@ config KRETPROBE_ON_RETHOOK
 	def_bool y
 	depends on HAVE_RETHOOK
 	depends on KRETPROBES
+	depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS || !HAVE_DYNAMIC_FTRACE_WITH_ARGS
 	select RETHOOK
 
This is the point why I said that "do not remove kretprobe trampoline".
If there is arch dependent kretprobe trampoline, kretprobe does not use
the rethook for hooking return. And eventually I would like to remove
kretprobe itself (replace it with fprobe + rethook). If so, I don't want
to pay more efforts on this part, and keep kretprobe on rethook as it is.

Thank you,

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

Re: [RFC PATCH v2 3/6] fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook

[Florent Revest]

On Wed, Aug 9, 2023 at 4:43 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > I think there are two things that can be meant with "rethook uses ftrace_regs":
> >
> > - rethook callbacks receive a ftrace_regs (that's what you do further down)
> > - rethook can hook to a traced function using a ftrace_regs (that's
> > what you use in fprobe now)
> >
> > But I think the second proposition shouldn't imply that rethook_hook
> > can _only_ hook to ftrace_regs. For the kprobe use case, I think there
> > should also be a rethook_hook_pt_regs() that operates on a pt_regs. We
> > could have a default implementation of rethook_hook that calls into
> > the other (or vice versa) on HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS
> > but I think it's good to separate these two APIs
>
> Yeah, so for simplying the 2nd case, I added this dependency.
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index aff2746c8af2..e321bdb8b22b 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -201,6 +201,7 @@ config KRETPROBE_ON_RETHOOK
>         def_bool y
>         depends on HAVE_RETHOOK
>         depends on KRETPROBES
> +       depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS || !HAVE_DYNAMIC_FTRACE_WITH_ARGS
>         select RETHOOK
>
> This is the point why I said that "do not remove kretprobe trampoline".
> If there is arch dependent kretprobe trampoline, kretprobe does not use
> the rethook for hooking return. And eventually I would like to remove
> kretprobe itself (replace it with fprobe + rethook). If so, I don't want
> to pay more efforts on this part, and keep kretprobe on rethook as it is.

What are your thoughts on kprobe + rethook though ?

If that's something you think is worth having, then in this case, it
seems that having a rethook_hook_pt_regs() API would help users.

If that's a frankenstein use case you don't want to support then I
agree we can live without this API and get away with the cast
protected by the depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS...

Re: [RFC PATCH v2 3/6] fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook

[Masami Hiramatsu]

On Wed, 9 Aug 2023 17:45:29 +0200
Florent Revest <revest@chromium.org> wrote:

> On Wed, Aug 9, 2023 at 4:43 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > > I think there are two things that can be meant with "rethook uses ftrace_regs":
> > >
> > > - rethook callbacks receive a ftrace_regs (that's what you do further down)
> > > - rethook can hook to a traced function using a ftrace_regs (that's
> > > what you use in fprobe now)
> > >
> > > But I think the second proposition shouldn't imply that rethook_hook
> > > can _only_ hook to ftrace_regs. For the kprobe use case, I think there
> > > should also be a rethook_hook_pt_regs() that operates on a pt_regs. We
> > > could have a default implementation of rethook_hook that calls into
> > > the other (or vice versa) on HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS
> > > but I think it's good to separate these two APIs
> >
> > Yeah, so for simplying the 2nd case, I added this dependency.
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index aff2746c8af2..e321bdb8b22b 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -201,6 +201,7 @@ config KRETPROBE_ON_RETHOOK
> >         def_bool y
> >         depends on HAVE_RETHOOK
> >         depends on KRETPROBES
> > +       depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS || !HAVE_DYNAMIC_FTRACE_WITH_ARGS
> >         select RETHOOK
> >
> > This is the point why I said that "do not remove kretprobe trampoline".
> > If there is arch dependent kretprobe trampoline, kretprobe does not use
> > the rethook for hooking return. And eventually I would like to remove
> > kretprobe itself (replace it with fprobe + rethook). If so, I don't want
> > to pay more efforts on this part, and keep kretprobe on rethook as it is.
> 
> What are your thoughts on kprobe + rethook though ?

Isn't it KRETPROBE_ON_RETHOOK?

> If that's something you think is worth having, then in this case, it
> seems that having a rethook_hook_pt_regs() API would help users.
> 
> If that's a frankenstein use case you don't want to support then I
> agree we can live without this API and get away with the cast
> protected by the depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS...

Yeah, it needs to introduce arch_rethook_prepare_pt_regs() for each
arch too.

BTW, I found that I have to update the implementation of
arch_rethook_prepare() for x86. (Use ftrace_get_stack_pointer())

Thank you!

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

[RFC PATCH v2 4/6] tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Allow fprobe events to be enabled with CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
With this change, fprobe events mostly use ftrace_regs instead of pt_regs.
Note that if the arch doesn't enable HAVE_PT_REGS_COMPAT_FTRACE_REGS,
fprobe events will not be able to use from perf.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/Kconfig            |    1 -
 kernel/trace/trace_fprobe.c     |   72 ++++++++++++++++++++++-----------------
 kernel/trace/trace_probe_tmpl.h |    2 +
 3 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 7d6abb5bd861..e9b7bd88cf9e 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -679,7 +679,6 @@ config FPROBE_EVENTS
 	select TRACING
 	select PROBE_EVENTS
 	select DYNAMIC_EVENTS
-	depends on DYNAMIC_FTRACE_WITH_REGS
 	default y
 	help
 	  This allows user to add tracing events on the function entry and
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index f440c97e050f..4e9c250dbd19 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -132,25 +132,30 @@ static int
 process_fetch_insn(struct fetch_insn *code, void *rec, void *dest,
 		   void *base)
 {
-	struct pt_regs *regs = rec;
-	unsigned long val;
+	struct ftrace_regs *fregs = rec;
+	unsigned long val, *stackp;
 	int ret;
 
 retry:
 	/* 1st stage: get value from context */
 	switch (code->op) {
 	case FETCH_OP_STACK:
-		val = regs_get_kernel_stack_nth(regs, code->param);
+		stackp = (unsigned long *)ftrace_regs_get_stack_pointer(fregs);
+		if (((unsigned long)(stackp + code->param) & ~(THREAD_SIZE - 1)) ==
+		    ((unsigned long)stackp & ~(THREAD_SIZE - 1)))
+			val = *(stackp + code->param);
+		else
+			val = 0;
 		break;
 	case FETCH_OP_STACKP:
-		val = kernel_stack_pointer(regs);
+		val = ftrace_regs_get_stack_pointer(fregs);
 		break;
 	case FETCH_OP_RETVAL:
-		val = regs_return_value(regs);
+		val = ftrace_regs_return_value(fregs);
 		break;
 #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
 	case FETCH_OP_ARG:
-		val = regs_get_kernel_argument(regs, code->param);
+		val = ftrace_regs_get_argument(fregs, code->param);
 		break;
 #endif
 	case FETCH_NOP_SYMBOL:	/* Ignore a place holder */
@@ -170,7 +175,7 @@ NOKPROBE_SYMBOL(process_fetch_insn)
 /* function entry handler */
 static nokprobe_inline void
 __fentry_trace_func(struct trace_fprobe *tf, unsigned long entry_ip,
-		    struct pt_regs *regs,
+		    struct ftrace_regs *fregs,
 		    struct trace_event_file *trace_file)
 {
 	struct fentry_trace_entry_head *entry;
@@ -184,36 +189,36 @@ __fentry_trace_func(struct trace_fprobe *tf, unsigned long entry_ip,
 	if (trace_trigger_soft_disabled(trace_file))
 		return;
 
-	dsize = __get_data_size(&tf->tp, regs);
+	dsize = __get_data_size(&tf->tp, fregs);
 
 	entry = trace_event_buffer_reserve(&fbuffer, trace_file,
 					   sizeof(*entry) + tf->tp.size + dsize);
 	if (!entry)
 		return;
 
-	fbuffer.regs = regs;
+	fbuffer.regs = ftrace_get_regs(fregs);
 	entry = fbuffer.entry = ring_buffer_event_data(fbuffer.event);
 	entry->ip = entry_ip;
-	store_trace_args(&entry[1], &tf->tp, regs, sizeof(*entry), dsize);
+	store_trace_args(&entry[1], &tf->tp, fregs, sizeof(*entry), dsize);
 
 	trace_event_buffer_commit(&fbuffer);
 }
 
 static void
 fentry_trace_func(struct trace_fprobe *tf, unsigned long entry_ip,
-		  struct pt_regs *regs)
+		  struct ftrace_regs *fregs)
 {
 	struct event_file_link *link;
 
 	trace_probe_for_each_link_rcu(link, &tf->tp)
-		__fentry_trace_func(tf, entry_ip, regs, link->file);
+		__fentry_trace_func(tf, entry_ip, fregs, link->file);
 }
 NOKPROBE_SYMBOL(fentry_trace_func);
 
 /* Kretprobe handler */
 static nokprobe_inline void
 __fexit_trace_func(struct trace_fprobe *tf, unsigned long entry_ip,
-		   unsigned long ret_ip, struct pt_regs *regs,
+		   unsigned long ret_ip, struct ftrace_regs *fregs,
 		   struct trace_event_file *trace_file)
 {
 	struct fexit_trace_entry_head *entry;
@@ -227,37 +232,37 @@ __fexit_trace_func(struct trace_fprobe *tf, unsigned long entry_ip,
 	if (trace_trigger_soft_disabled(trace_file))
 		return;
 
-	dsize = __get_data_size(&tf->tp, regs);
+	dsize = __get_data_size(&tf->tp, fregs);
 
 	entry = trace_event_buffer_reserve(&fbuffer, trace_file,
 					   sizeof(*entry) + tf->tp.size + dsize);
 	if (!entry)
 		return;
 
-	fbuffer.regs = regs;
+	fbuffer.regs = ftrace_get_regs(fregs);
 	entry = fbuffer.entry = ring_buffer_event_data(fbuffer.event);
 	entry->func = entry_ip;
 	entry->ret_ip = ret_ip;
-	store_trace_args(&entry[1], &tf->tp, regs, sizeof(*entry), dsize);
+	store_trace_args(&entry[1], &tf->tp, fregs, sizeof(*entry), dsize);
 
 	trace_event_buffer_commit(&fbuffer);
 }
 
 static void
 fexit_trace_func(struct trace_fprobe *tf, unsigned long entry_ip,
-		 unsigned long ret_ip, struct pt_regs *regs)
+		 unsigned long ret_ip, struct ftrace_regs *fregs)
 {
 	struct event_file_link *link;
 
 	trace_probe_for_each_link_rcu(link, &tf->tp)
-		__fexit_trace_func(tf, entry_ip, ret_ip, regs, link->file);
+		__fexit_trace_func(tf, entry_ip, ret_ip, fregs, link->file);
 }
 NOKPROBE_SYMBOL(fexit_trace_func);
 
 #ifdef CONFIG_PERF_EVENTS
 
 static int fentry_perf_func(struct trace_fprobe *tf, unsigned long entry_ip,
-			    struct pt_regs *regs)
+			    struct ftrace_regs *fregs, struct pt_regs *regs)
 {
 	struct trace_event_call *call = trace_probe_event_call(&tf->tp);
 	struct fentry_trace_entry_head *entry;
@@ -269,7 +274,7 @@ static int fentry_perf_func(struct trace_fprobe *tf, unsigned long entry_ip,
 	if (hlist_empty(head))
 		return 0;
 
-	dsize = __get_data_size(&tf->tp, regs);
+	dsize = __get_data_size(&tf->tp, fregs);
 	__size = sizeof(*entry) + tf->tp.size + dsize;
 	size = ALIGN(__size + sizeof(u32), sizeof(u64));
 	size -= sizeof(u32);
@@ -280,7 +285,7 @@ static int fentry_perf_func(struct trace_fprobe *tf, unsigned long entry_ip,
 
 	entry->ip = entry_ip;
 	memset(&entry[1], 0, dsize);
-	store_trace_args(&entry[1], &tf->tp, regs, sizeof(*entry), dsize);
+	store_trace_args(&entry[1], &tf->tp, fregs, sizeof(*entry), dsize);
 	perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
 			      head, NULL);
 	return 0;
@@ -289,7 +294,8 @@ NOKPROBE_SYMBOL(fentry_perf_func);
 
 static void
 fexit_perf_func(struct trace_fprobe *tf, unsigned long entry_ip,
-		unsigned long ret_ip, struct pt_regs *regs)
+		unsigned long ret_ip, struct ftrace_regs *fregs,
+		struct pt_regs *regs)
 {
 	struct trace_event_call *call = trace_probe_event_call(&tf->tp);
 	struct fexit_trace_entry_head *entry;
@@ -301,7 +307,7 @@ fexit_perf_func(struct trace_fprobe *tf, unsigned long entry_ip,
 	if (hlist_empty(head))
 		return;
 
-	dsize = __get_data_size(&tf->tp, regs);
+	dsize = __get_data_size(&tf->tp, fregs);
 	__size = sizeof(*entry) + tf->tp.size + dsize;
 	size = ALIGN(__size + sizeof(u32), sizeof(u64));
 	size -= sizeof(u32);
@@ -312,7 +318,7 @@ fexit_perf_func(struct trace_fprobe *tf, unsigned long entry_ip,
 
 	entry->func = entry_ip;
 	entry->ret_ip = ret_ip;
-	store_trace_args(&entry[1], &tf->tp, regs, sizeof(*entry), dsize);
+	store_trace_args(&entry[1], &tf->tp, fregs, sizeof(*entry), dsize);
 	perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
 			      head, NULL);
 }
@@ -327,14 +333,15 @@ static int fentry_dispatcher(struct fprobe *fp, unsigned long entry_ip,
 	struct pt_regs *regs = ftrace_get_regs(fregs);
 	int ret = 0;
 
+	if (trace_probe_test_flag(&tf->tp, TP_FLAG_TRACE))
+		fentry_trace_func(tf, entry_ip, fregs);
+
+#ifdef CONFIG_PERF_EVENTS
 	if (!regs)
 		return 0;
 
-	if (trace_probe_test_flag(&tf->tp, TP_FLAG_TRACE))
-		fentry_trace_func(tf, entry_ip, regs);
-#ifdef CONFIG_PERF_EVENTS
 	if (trace_probe_test_flag(&tf->tp, TP_FLAG_PROFILE))
-		ret = fentry_perf_func(tf, entry_ip, regs);
+		ret = fentry_perf_func(tf, entry_ip, fregs, regs);
 #endif
 	return ret;
 }
@@ -347,14 +354,15 @@ static void fexit_dispatcher(struct fprobe *fp, unsigned long entry_ip,
 	struct trace_fprobe *tf = container_of(fp, struct trace_fprobe, fp);
 	struct pt_regs *regs = ftrace_get_regs(fregs);
 
+	if (trace_probe_test_flag(&tf->tp, TP_FLAG_TRACE))
+		fexit_trace_func(tf, entry_ip, ret_ip, fregs);
+
+#ifdef CONFIG_PERF_EVENTS
 	if (!regs)
 		return;
 
-	if (trace_probe_test_flag(&tf->tp, TP_FLAG_TRACE))
-		fexit_trace_func(tf, entry_ip, ret_ip, regs);
-#ifdef CONFIG_PERF_EVENTS
 	if (trace_probe_test_flag(&tf->tp, TP_FLAG_PROFILE))
-		fexit_perf_func(tf, entry_ip, ret_ip, regs);
+		fexit_perf_func(tf, entry_ip, ret_ip, fregs, regs);
 #endif
 }
 NOKPROBE_SYMBOL(fexit_dispatcher);
diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index 3935b347f874..05445a745a07 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -232,7 +232,7 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
 
 /* Sum up total data length for dynamic arrays (strings) */
 static nokprobe_inline int
-__get_data_size(struct trace_probe *tp, struct pt_regs *regs)
+__get_data_size(struct trace_probe *tp, void *regs)
 {
 	struct probe_arg *arg;
 	int i, len, ret = 0;

Re: [RFC PATCH v2 4/6] tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Florent Revest]

On Mon, Aug 7, 2023 at 8:49 AM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Allow fprobe events to be enabled with CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
> With this change, fprobe events mostly use ftrace_regs instead of pt_regs.
> Note that if the arch doesn't enable HAVE_PT_REGS_COMPAT_FTRACE_REGS,
> fprobe events will not be able to use from perf.

nit: "to be used from perf" ?

> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -132,25 +132,30 @@ static int
>  process_fetch_insn(struct fetch_insn *code, void *rec, void *dest,
>                    void *base)
>  {
> -       struct pt_regs *regs = rec;
> -       unsigned long val;
> +       struct ftrace_regs *fregs = rec;
> +       unsigned long val, *stackp;
>         int ret;
>
>  retry:
>         /* 1st stage: get value from context */
>         switch (code->op) {
>         case FETCH_OP_STACK:
> -               val = regs_get_kernel_stack_nth(regs, code->param);
> +               stackp = (unsigned long *)ftrace_regs_get_stack_pointer(fregs);
> +               if (((unsigned long)(stackp + code->param) & ~(THREAD_SIZE - 1)) ==
> +                   ((unsigned long)stackp & ~(THREAD_SIZE - 1)))

Maybe it'd be worth extracting a local
"ftrace_regs_get_kernel_stack_nth_addr" helper function and/or
"ftrace_regs_within_kernel_stack" ?

Re: [RFC PATCH v2 4/6] tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Masami Hiramatsu]

On Wed, 9 Aug 2023 12:31:00 +0200
Florent Revest <revest@chromium.org> wrote:

> On Mon, Aug 7, 2023 at 8:49 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Allow fprobe events to be enabled with CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
> > With this change, fprobe events mostly use ftrace_regs instead of pt_regs.
> > Note that if the arch doesn't enable HAVE_PT_REGS_COMPAT_FTRACE_REGS,
> > fprobe events will not be able to use from perf.
> 
> nit: "to be used from perf" ?

OK.

> 
> > --- a/kernel/trace/trace_fprobe.c
> > +++ b/kernel/trace/trace_fprobe.c
> > @@ -132,25 +132,30 @@ static int
> >  process_fetch_insn(struct fetch_insn *code, void *rec, void *dest,
> >                    void *base)
> >  {
> > -       struct pt_regs *regs = rec;
> > -       unsigned long val;
> > +       struct ftrace_regs *fregs = rec;
> > +       unsigned long val, *stackp;
> >         int ret;
> >
> >  retry:
> >         /* 1st stage: get value from context */
> >         switch (code->op) {
> >         case FETCH_OP_STACK:
> > -               val = regs_get_kernel_stack_nth(regs, code->param);
> > +               stackp = (unsigned long *)ftrace_regs_get_stack_pointer(fregs);
> > +               if (((unsigned long)(stackp + code->param) & ~(THREAD_SIZE - 1)) ==
> > +                   ((unsigned long)stackp & ~(THREAD_SIZE - 1)))
> 
> Maybe it'd be worth extracting a local
> "ftrace_regs_get_kernel_stack_nth_addr" helper function and/or
> "ftrace_regs_within_kernel_stack" ?

Yeah, maybe we can make it a generic inline function in linux/ftrace.h.

Thank you!


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

Re: [RFC PATCH v2 4/6] tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Florent Revest]

On Wed, Aug 9, 2023 at 4:45 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > --- a/kernel/trace/trace_fprobe.c
> > > +++ b/kernel/trace/trace_fprobe.c
> > > @@ -132,25 +132,30 @@ static int
> > >  process_fetch_insn(struct fetch_insn *code, void *rec, void *dest,
> > >                    void *base)
> > >  {
> > > -       struct pt_regs *regs = rec;
> > > -       unsigned long val;
> > > +       struct ftrace_regs *fregs = rec;
> > > +       unsigned long val, *stackp;
> > >         int ret;
> > >
> > >  retry:
> > >         /* 1st stage: get value from context */
> > >         switch (code->op) {
> > >         case FETCH_OP_STACK:
> > > -               val = regs_get_kernel_stack_nth(regs, code->param);
> > > +               stackp = (unsigned long *)ftrace_regs_get_stack_pointer(fregs);
> > > +               if (((unsigned long)(stackp + code->param) & ~(THREAD_SIZE - 1)) ==
> > > +                   ((unsigned long)stackp & ~(THREAD_SIZE - 1)))
> >
> > Maybe it'd be worth extracting a local
> > "ftrace_regs_get_kernel_stack_nth_addr" helper function and/or
> > "ftrace_regs_within_kernel_stack" ?
>
> Yeah, maybe we can make it a generic inline function in linux/ftrace.h.

Or even just above this function if there are low chances it would get
used elsewhere :)

Re: [RFC PATCH v2 4/6] tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Masami Hiramatsu]

On Wed, 9 Aug 2023 17:38:00 +0200
Florent Revest <revest@chromium.org> wrote:

> On Wed, Aug 9, 2023 at 4:45 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > > --- a/kernel/trace/trace_fprobe.c
> > > > +++ b/kernel/trace/trace_fprobe.c
> > > > @@ -132,25 +132,30 @@ static int
> > > >  process_fetch_insn(struct fetch_insn *code, void *rec, void *dest,
> > > >                    void *base)
> > > >  {
> > > > -       struct pt_regs *regs = rec;
> > > > -       unsigned long val;
> > > > +       struct ftrace_regs *fregs = rec;
> > > > +       unsigned long val, *stackp;
> > > >         int ret;
> > > >
> > > >  retry:
> > > >         /* 1st stage: get value from context */
> > > >         switch (code->op) {
> > > >         case FETCH_OP_STACK:
> > > > -               val = regs_get_kernel_stack_nth(regs, code->param);
> > > > +               stackp = (unsigned long *)ftrace_regs_get_stack_pointer(fregs);
> > > > +               if (((unsigned long)(stackp + code->param) & ~(THREAD_SIZE - 1)) ==
> > > > +                   ((unsigned long)stackp & ~(THREAD_SIZE - 1)))
> > >
> > > Maybe it'd be worth extracting a local
> > > "ftrace_regs_get_kernel_stack_nth_addr" helper function and/or
> > > "ftrace_regs_within_kernel_stack" ?
> >
> > Yeah, maybe we can make it a generic inline function in linux/ftrace.h.
> 
> Or even just above this function if there are low chances it would get
> used elsewhere :)

Thanks, but since regs_get_kernel_stack_nth() is defined in asm/ptrace.h,
I think ftrace_regs_get_kernel_stack_nth() is better defined in
linux/ftrace.h. :)

Thank you!

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

Re: [RFC PATCH v2 4/6] tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Steven Rostedt]

On Thu, 10 Aug 2023 09:38:45 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > Or even just above this function if there are low chances it would get
> > used elsewhere :)  
> 
> Thanks, but since regs_get_kernel_stack_nth() is defined in asm/ptrace.h,
> I think ftrace_regs_get_kernel_stack_nth() is better defined in
> linux/ftrace.h. :)

I agree with Masami.

Thanks,

-- Steve

[RFC PATCH v2 5/6] ftrace: Add ftrace_partial_regs() for converting ftrace_regs to pt_regs

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add ftrace_partial_regs() which converts the ftrace_regas to pt_regs.
If the architecture defines its own ftrace_regs, this copies partial
registers to pt_regs and returns it. If not, ftrace_regs is the same as
pt_regs and ftrace_partial_regs() will return ftrace_regs::regs.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 arch/arm64/include/asm/ftrace.h |   11 +++++++++++
 include/linux/ftrace.h          |   11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index ab158196480c..b108cd6718cf 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -137,6 +137,17 @@ ftrace_override_function_with_return(struct ftrace_regs *fregs)
 	fregs->pc = fregs->lr;
 }
 
+static __always_inline struct pt_regs *
+ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
+{
+	memcpy(regs->regs, fregs->regs, sizeof(u64) * 10);
+	regs->sp = fregs->sp;
+	regs->pc = fregs->pc;
+	regs->x[29] = fregs->fp;
+	regs->x[30] = fregs->lr;
+	return regs;
+}
+
 int ftrace_regs_query_register_offset(const char *name);
 
 int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 3fb94a1a2461..7f45654441b7 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -155,6 +155,17 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
 	return arch_ftrace_get_regs(fregs);
 }
 
+#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || \
+	defined(CONFIG_HAVE_PT_REGS_COMPAT_FTRACE_REGS)
+
+static __always_inline struct pt_regs *
+ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
+{
+	return arch_ftrace_get_regs((struct ftrace_regs *)fregs);
+}
+
+#endif /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS || CONFIG_HAVE_PT_REGS_COMPAT_FTRACE_REGS */
+
 /*
  * When true, the ftrace_regs_{get,set}_*() functions may be used on fregs.
  * Note: this can be true even when ftrace_get_regs() cannot provide a pt_regs.

Re: [RFC PATCH v2 5/6] ftrace: Add ftrace_partial_regs() for converting ftrace_regs to pt_regs

[Florent Revest]

On Mon, Aug 7, 2023 at 8:49 AM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Add ftrace_partial_regs() which converts the ftrace_regas to pt_regs.

ftrace_regs*

> If the architecture defines its own ftrace_regs, this copies partial
> registers to pt_regs and returns it. If not, ftrace_regs is the same as
> pt_regs and ftrace_partial_regs() will return ftrace_regs::regs.
>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  arch/arm64/include/asm/ftrace.h |   11 +++++++++++
>  include/linux/ftrace.h          |   11 +++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index ab158196480c..b108cd6718cf 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -137,6 +137,17 @@ ftrace_override_function_with_return(struct ftrace_regs *fregs)
>         fregs->pc = fregs->lr;
>  }
>
> +static __always_inline struct pt_regs *
> +ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
> +{
> +       memcpy(regs->regs, fregs->regs, sizeof(u64) * 10);

Are you intentionally copying that tenth value (fregs.direct_tramp)
into pt_regs.regs[9] ? This seems wrong and it looks like it will bite
us back one day. Isn't it one of these cases where we can simply use
sizeof(fregs->regs) ?

> +       regs->sp = fregs->sp;
> +       regs->pc = fregs->pc;
> +       regs->x[29] = fregs->fp;
> +       regs->x[30] = fregs->lr;
> +       return regs;
> +}
> +
>  int ftrace_regs_query_register_offset(const char *name);
>
>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 3fb94a1a2461..7f45654441b7 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -155,6 +155,17 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
>         return arch_ftrace_get_regs(fregs);
>  }
>
> +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || \
> +       defined(CONFIG_HAVE_PT_REGS_COMPAT_FTRACE_REGS)
> +
> +static __always_inline struct pt_regs *
> +ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
> +{
> +       return arch_ftrace_get_regs((struct ftrace_regs *)fregs);
> +}

I don't think this works. Suppose you are on x86, WITH_ARGS, and with
HAVE_PT_REGS_COMPAT_FTRACE_REGS. If you register to ftrace without
FTRACE_OPS_FL_SAVE_REGS you will receive a ftrace_regs from the light
ftrace pre-trampoline that has a CS register equal to 0 and
arch_ftrace_get_regs will return NULL here, which should never happen.

Have you tested your series without registering as FTRACE_OPS_FL_SAVE_REGS ?

Re: [RFC PATCH v2 5/6] ftrace: Add ftrace_partial_regs() for converting ftrace_regs to pt_regs

[Masami Hiramatsu]

On Wed, 9 Aug 2023 12:31:27 +0200
Florent Revest <revest@chromium.org> wrote:

> On Mon, Aug 7, 2023 at 8:49 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Add ftrace_partial_regs() which converts the ftrace_regas to pt_regs.
> 
> ftrace_regs*

Oops, thanks.

> 
> > If the architecture defines its own ftrace_regs, this copies partial
> > registers to pt_regs and returns it. If not, ftrace_regs is the same as
> > pt_regs and ftrace_partial_regs() will return ftrace_regs::regs.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  arch/arm64/include/asm/ftrace.h |   11 +++++++++++
> >  include/linux/ftrace.h          |   11 +++++++++++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> > index ab158196480c..b108cd6718cf 100644
> > --- a/arch/arm64/include/asm/ftrace.h
> > +++ b/arch/arm64/include/asm/ftrace.h
> > @@ -137,6 +137,17 @@ ftrace_override_function_with_return(struct ftrace_regs *fregs)
> >         fregs->pc = fregs->lr;
> >  }
> >
> > +static __always_inline struct pt_regs *
> > +ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
> > +{
> > +       memcpy(regs->regs, fregs->regs, sizeof(u64) * 10);
> 
> Are you intentionally copying that tenth value (fregs.direct_tramp)
> into pt_regs.regs[9] ? This seems wrong and it looks like it will bite
> us back one day. Isn't it one of these cases where we can simply use
> sizeof(fregs->regs) ?

Ah, sorry, it was my mistake. It should be "sizeof(u64) * 9".
I would like to know how can I handle the 'direct_tramp' thing?
Can I just ignore it?

> 
> > +       regs->sp = fregs->sp;
> > +       regs->pc = fregs->pc;
> > +       regs->x[29] = fregs->fp;
> > +       regs->x[30] = fregs->lr;
> > +       return regs;
> > +}
> > +
> >  int ftrace_regs_query_register_offset(const char *name);
> >
> >  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 3fb94a1a2461..7f45654441b7 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -155,6 +155,17 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
> >         return arch_ftrace_get_regs(fregs);
> >  }
> >
> > +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || \
> > +       defined(CONFIG_HAVE_PT_REGS_COMPAT_FTRACE_REGS)
> > +
> > +static __always_inline struct pt_regs *
> > +ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
> > +{
> > +       return arch_ftrace_get_regs((struct ftrace_regs *)fregs);
> > +}
> 
> I don't think this works. Suppose you are on x86, WITH_ARGS, and with
> HAVE_PT_REGS_COMPAT_FTRACE_REGS. If you register to ftrace without
> FTRACE_OPS_FL_SAVE_REGS you will receive a ftrace_regs from the light
> ftrace pre-trampoline that has a CS register equal to 0 and
> arch_ftrace_get_regs will return NULL here, which should never happen.

Yes, Jiri also pointed it. So I simply made it (also remove 'const' from fregs)

return &fregs->regs;

Thank you,

> 
> Have you tested your series without registering as FTRACE_OPS_FL_SAVE_REGS ?

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

[RFC PATCH v2 6/6] bpf: Enable kprobe_multi feature if CONFIG_FPROBE is enabled

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Enable kprobe_multi feature if CONFIG_FPROBE is enabled. The pt_regs is
converted from ftrace_regs by ftrace_partial_regs(), thus some registers
may always returns 0. But it should be enough for function entry (access
arguments) and exit (access return value).

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/bpf_trace.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 99c5f95360f9..0725272a3de2 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2460,7 +2460,7 @@ static int __init bpf_event_init(void)
 fs_initcall(bpf_event_init);
 #endif /* CONFIG_MODULES */
 
-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#ifdef CONFIG_FPROBE
 struct bpf_kprobe_multi_link {
 	struct bpf_link link;
 	struct fprobe fp;
@@ -2482,6 +2482,8 @@ struct user_syms {
 	char *buf;
 };
 
+static DEFINE_PER_CPU(struct pt_regs, bpf_kprobe_multi_pt_regs);
+
 static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 cnt)
 {
 	unsigned long __user usymbol;
@@ -2623,13 +2625,14 @@ static u64 bpf_kprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
 
 static int
 kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
-			   unsigned long entry_ip, struct pt_regs *regs)
+			   unsigned long entry_ip, struct ftrace_regs *fregs)
 {
 	struct bpf_kprobe_multi_run_ctx run_ctx = {
 		.link = link,
 		.entry_ip = entry_ip,
 	};
 	struct bpf_run_ctx *old_run_ctx;
+	struct pt_regs *regs;
 	int err;
 
 	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
@@ -2639,6 +2642,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 
 	migrate_disable();
 	rcu_read_lock();
+	regs = ftrace_partial_regs(fregs, this_cpu_ptr(&bpf_kprobe_multi_pt_regs));
 	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
 	err = bpf_prog_run(link->link.prog, regs);
 	bpf_reset_run_ctx(old_run_ctx);
@@ -2656,13 +2660,9 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
 			  void *data)
 {
 	struct bpf_kprobe_multi_link *link;
-	struct pt_regs *regs = ftrace_get_regs(fregs);
-
-	if (!regs)
-		return 0;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
+	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), fregs);
 	return 0;
 }
 
@@ -2672,13 +2672,9 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
 			       void *data)
 {
 	struct bpf_kprobe_multi_link *link;
-	struct pt_regs *regs = ftrace_get_regs(fregs);
-
-	if (!regs)
-		return;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
+	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), fregs);
 }
 
 static int symbols_cmp_r(const void *a, const void *b, const void *priv)
@@ -2918,7 +2914,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	kvfree(cookies);
 	return err;
 }
-#else /* !CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+#else /* !CONFIG_FPROBE */
 int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
 	return -EOPNOTSUPP;

Re: [RFC PATCH v2 6/6] bpf: Enable kprobe_multi feature if CONFIG_FPROBE is enabled

[Jiri Olsa]

On Mon, Aug 07, 2023 at 03:49:33PM +0900, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Enable kprobe_multi feature if CONFIG_FPROBE is enabled. The pt_regs is
> converted from ftrace_regs by ftrace_partial_regs(), thus some registers
> may always returns 0. But it should be enough for function entry (access
> arguments) and exit (access return value).
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/bpf_trace.c |   22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 99c5f95360f9..0725272a3de2 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2460,7 +2460,7 @@ static int __init bpf_event_init(void)
>  fs_initcall(bpf_event_init);
>  #endif /* CONFIG_MODULES */
>  
> -#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +#ifdef CONFIG_FPROBE
>  struct bpf_kprobe_multi_link {
>  	struct bpf_link link;
>  	struct fprobe fp;
> @@ -2482,6 +2482,8 @@ struct user_syms {
>  	char *buf;
>  };
>  
> +static DEFINE_PER_CPU(struct pt_regs, bpf_kprobe_multi_pt_regs);
> +
>  static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 cnt)
>  {
>  	unsigned long __user usymbol;
> @@ -2623,13 +2625,14 @@ static u64 bpf_kprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
>  
>  static int
>  kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> -			   unsigned long entry_ip, struct pt_regs *regs)
> +			   unsigned long entry_ip, struct ftrace_regs *fregs)
>  {
>  	struct bpf_kprobe_multi_run_ctx run_ctx = {
>  		.link = link,
>  		.entry_ip = entry_ip,
>  	};
>  	struct bpf_run_ctx *old_run_ctx;
> +	struct pt_regs *regs;
>  	int err;
>  
>  	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> @@ -2639,6 +2642,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
>  
>  	migrate_disable();
>  	rcu_read_lock();
> +	regs = ftrace_partial_regs(fregs, this_cpu_ptr(&bpf_kprobe_multi_pt_regs));

you did check for !regs when returned from ftrace_get_regs, why don't we need
to check it in here? both ftrace_partial_regs and ftrace_get_regs call
arch_ftrace_get_regs on x86

also also I can't find the place ensuring fregs->regs.cs != 0 for FL_SAVE_REGS
flag as stated in arch_ftrace_get_regs, any hint?

thanks,
jirka


>  	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
>  	err = bpf_prog_run(link->link.prog, regs);
>  	bpf_reset_run_ctx(old_run_ctx);
> @@ -2656,13 +2660,9 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
>  			  void *data)
>  {
>  	struct bpf_kprobe_multi_link *link;
> -	struct pt_regs *regs = ftrace_get_regs(fregs);
> -
> -	if (!regs)
> -		return 0;
>  
>  	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> -	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> +	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), fregs);
>  	return 0;
>  }
>  
> @@ -2672,13 +2672,9 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
>  			       void *data)
>  {
>  	struct bpf_kprobe_multi_link *link;
> -	struct pt_regs *regs = ftrace_get_regs(fregs);
> -
> -	if (!regs)
> -		return;
>  
>  	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> -	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> +	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), fregs);
>  }
>  
>  static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> @@ -2918,7 +2914,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  	kvfree(cookies);
>  	return err;
>  }
> -#else /* !CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> +#else /* !CONFIG_FPROBE */
>  int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  {
>  	return -EOPNOTSUPP;
> 

Re: [RFC PATCH v2 6/6] bpf: Enable kprobe_multi feature if CONFIG_FPROBE is enabled

[Masami Hiramatsu]

On Tue, 8 Aug 2023 00:08:11 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:

> On Mon, Aug 07, 2023 at 03:49:33PM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Enable kprobe_multi feature if CONFIG_FPROBE is enabled. The pt_regs is
> > converted from ftrace_regs by ftrace_partial_regs(), thus some registers
> > may always returns 0. But it should be enough for function entry (access
> > arguments) and exit (access return value).
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c |   22 +++++++++-------------
> >  1 file changed, 9 insertions(+), 13 deletions(-)
> > 
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 99c5f95360f9..0725272a3de2 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2460,7 +2460,7 @@ static int __init bpf_event_init(void)
> >  fs_initcall(bpf_event_init);
> >  #endif /* CONFIG_MODULES */
> >  
> > -#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > +#ifdef CONFIG_FPROBE
> >  struct bpf_kprobe_multi_link {
> >  	struct bpf_link link;
> >  	struct fprobe fp;
> > @@ -2482,6 +2482,8 @@ struct user_syms {
> >  	char *buf;
> >  };
> >  
> > +static DEFINE_PER_CPU(struct pt_regs, bpf_kprobe_multi_pt_regs);
> > +
> >  static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 cnt)
> >  {
> >  	unsigned long __user usymbol;
> > @@ -2623,13 +2625,14 @@ static u64 bpf_kprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
> >  
> >  static int
> >  kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> > -			   unsigned long entry_ip, struct pt_regs *regs)
> > +			   unsigned long entry_ip, struct ftrace_regs *fregs)
> >  {
> >  	struct bpf_kprobe_multi_run_ctx run_ctx = {
> >  		.link = link,
> >  		.entry_ip = entry_ip,
> >  	};
> >  	struct bpf_run_ctx *old_run_ctx;
> > +	struct pt_regs *regs;
> >  	int err;
> >  
> >  	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> > @@ -2639,6 +2642,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> >  
> >  	migrate_disable();
> >  	rcu_read_lock();
> > +	regs = ftrace_partial_regs(fregs, this_cpu_ptr(&bpf_kprobe_multi_pt_regs));
> 
> you did check for !regs when returned from ftrace_get_regs, why don't we need
> to check it in here? both ftrace_partial_regs and ftrace_get_regs call
> arch_ftrace_get_regs on x86

Good catch! I think ftrace_partial_regs must not return NULL (unless getting
invalid parameter, e.g. fregs == NULL).

> 
> also also I can't find the place ensuring fregs->regs.cs != 0 for FL_SAVE_REGS
> flag as stated in arch_ftrace_get_regs, any hint?

Oops, I misread that part. Maybe ftrace_partial_regs must forcibly return
ftrace_regs::regs if HAVE_PT_REGS_COMPAT_FTRACE_REGS=y because it does not
care the regs is partial or not.

Thank you!

> 
> thanks,
> jirka
> 
> 
> >  	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
> >  	err = bpf_prog_run(link->link.prog, regs);
> >  	bpf_reset_run_ctx(old_run_ctx);
> > @@ -2656,13 +2660,9 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
> >  			  void *data)
> >  {
> >  	struct bpf_kprobe_multi_link *link;
> > -	struct pt_regs *regs = ftrace_get_regs(fregs);
> > -
> > -	if (!regs)
> > -		return 0;
> >  
> >  	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > -	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> > +	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), fregs);
> >  	return 0;
> >  }
> >  
> > @@ -2672,13 +2672,9 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
> >  			       void *data)
> >  {
> >  	struct bpf_kprobe_multi_link *link;
> > -	struct pt_regs *regs = ftrace_get_regs(fregs);
> > -
> > -	if (!regs)
> > -		return;
> >  
> >  	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > -	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> > +	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), fregs);
> >  }
> >  
> >  static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > @@ -2918,7 +2914,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >  	kvfree(cookies);
> >  	return err;
> >  }
> > -#else /* !CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> > +#else /* !CONFIG_FPROBE */
> >  int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> >  {
> >  	return -EOPNOTSUPP;
> > 


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

Re: [RFC PATCH v2 0/6] bpf: fprobe: rethook: Use ftrace_regs instead of pt_regs

[Florent Revest]

On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> Florent, feel free to add your rethook for arm64, but please do not remove
> kretprobe trampoline yet. It is another discussion point. We may be possible
> to use ftrace_regs for kretprobe by ftrace_partial_regs() but kretprobe
> allows nest probe. (maybe we can skip that case?)

Ack :)

>  arch/Kconfig                    |    1 +
>  arch/arm64/include/asm/ftrace.h |   11 ++++++
>  arch/loongarch/Kconfig          |    1 +
>  arch/s390/Kconfig               |    1 +
>  arch/x86/Kconfig                |    1 +
>  arch/x86/kernel/rethook.c       |    9 +++--
>  include/linux/fprobe.h          |    4 +-
>  include/linux/ftrace.h          |   56 ++++++++++++++++++-----------
>  include/linux/rethook.h         |   11 +++---
>  kernel/kprobes.c                |    9 ++++-
>  kernel/trace/Kconfig            |    9 ++++-
>  kernel/trace/bpf_trace.c        |   14 +++++--
>  kernel/trace/fprobe.c           |    8 ++--
>  kernel/trace/rethook.c          |   16 ++++----
>  kernel/trace/trace_fprobe.c     |   76 ++++++++++++++++++++++++---------------
>  kernel/trace/trace_probe_tmpl.h |    2 +
>  lib/test_fprobe.c               |   10 +++--
>  samples/fprobe/fprobe_example.c |    4 +-

I believe that Documentation/trace/fprobe.rst should also be modified
following the API change

Re: [RFC PATCH v2 0/6] bpf: fprobe: rethook: Use ftrace_regs instead of pt_regs

[Masami Hiramatsu]

On Tue, 8 Aug 2023 16:29:27 +0200
Florent Revest <revest@chromium.org> wrote:

> On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > Florent, feel free to add your rethook for arm64, but please do not remove
> > kretprobe trampoline yet. It is another discussion point. We may be possible
> > to use ftrace_regs for kretprobe by ftrace_partial_regs() but kretprobe
> > allows nest probe. (maybe we can skip that case?)
> 
> Ack :)
> 
> >  arch/Kconfig                    |    1 +
> >  arch/arm64/include/asm/ftrace.h |   11 ++++++
> >  arch/loongarch/Kconfig          |    1 +
> >  arch/s390/Kconfig               |    1 +
> >  arch/x86/Kconfig                |    1 +
> >  arch/x86/kernel/rethook.c       |    9 +++--
> >  include/linux/fprobe.h          |    4 +-
> >  include/linux/ftrace.h          |   56 ++++++++++++++++++-----------
> >  include/linux/rethook.h         |   11 +++---
> >  kernel/kprobes.c                |    9 ++++-
> >  kernel/trace/Kconfig            |    9 ++++-
> >  kernel/trace/bpf_trace.c        |   14 +++++--
> >  kernel/trace/fprobe.c           |    8 ++--
> >  kernel/trace/rethook.c          |   16 ++++----
> >  kernel/trace/trace_fprobe.c     |   76 ++++++++++++++++++++++++---------------
> >  kernel/trace/trace_probe_tmpl.h |    2 +
> >  lib/test_fprobe.c               |   10 +++--
> >  samples/fprobe/fprobe_example.c |    4 +-
> 
> I believe that Documentation/trace/fprobe.rst should also be modified
> following the API change

Indeed. Let me update it.

Thanks!


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