diff mbox series

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

Message ID 169139095066.324433.15514499924371317690.stgit@devnote2 (mailing list archive)
State Superseded
Headers show
Series bpf: fprobe: rethook: Use ftrace_regs instead of pt_regs | expand

Commit Message

Masami Hiramatsu (Google) Aug. 7, 2023, 6:49 a.m. UTC 
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(-)

Comments

Florent Revest Aug. 9, 2023, 10:31 a.m. UTC | #1 
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" ?
Masami Hiramatsu (Google) Aug. 9, 2023, 2:45 p.m. UTC | #2 
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!
Florent Revest Aug. 9, 2023, 3:38 p.m. UTC | #3 
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 :)
Masami Hiramatsu (Google) Aug. 10, 2023, 12:38 a.m. UTC | #4 
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!
Steven Rostedt Aug. 11, 2023, 3:57 p.m. UTC | #5 
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
diff mbox series

Patch

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;