Received-SPF: pass (google.com: domain of linux-kernel+bounces-6070-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 20 Dec 2023 07:51:59 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Jiri Olsa <olsajiri@gmail.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>, Steven Rostedt
 <rostedt@goodmis.org>, Florent Revest <revest@chromium.org>,
 linux-trace-kernel@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
 Martin KaFai Lau <martin.lau@linux.dev>, bpf <bpf@vger.kernel.org>, Sven
 Schnelle <svens@linux.ibm.com>, Alexei Starovoitov <ast@kernel.org>,
 Arnaldo Carvalho de Melo <acme@kernel.org>, Daniel Borkmann
 <daniel@iogearbox.net>, Alan Maguire <alan.maguire@oracle.com>, Mark
 Rutland <mark.rutland@arm.com>, Peter Zijlstra <peterz@infradead.org>,
 Thomas Gleixner <tglx@linutronix.de>, Guo Ren <guoren@kernel.org>
Subject: Re: [PATCH v5 24/34] fprobe: Use ftrace_regs in fprobe entry
 handler
Message-Id: <20231220075159.a999e25dbf56f0334ba79249@kernel.org>
In-Reply-To: <ZYGZZES--JmqQN_v@krava>
References: <170290509018.220107.1347127510564358608.stgit@devnote2>
	<170290538307.220107.14964448383069008953.stgit@devnote2>
	<ZYGZZES--JmqQN_v@krava>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 19 Dec 2023 14:23:48 +0100
Jiri Olsa <olsajiri@gmail.com> wrote:

> On Mon, Dec 18, 2023 at 10:16:23PM +0900, Masami Hiramatsu (Google) wrote:
> 
> SNIP
> 
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 84e8a0f6e4e0..d3f8745d8ead 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2503,7 +2503,7 @@ static int __init bpf_event_init(void)
> >  fs_initcall(bpf_event_init);
> >  #endif /* CONFIG_MODULES */
> >  
> > -#ifdef CONFIG_FPROBE
> > +#if defined(CONFIG_FPROBE) && defined(CONFIG_DYNAMIC_FTRACE_WITH_REGS)
> >  struct bpf_kprobe_multi_link {
> >  	struct bpf_link link;
> >  	struct fprobe fp;
> > @@ -2733,10 +2733,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);
> > @@ -3008,7 +3012,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_FPROBE || !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 6cd2a4e3afb8..f12569494d8a 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) {
> > @@ -182,7 +182,7 @@ static void fprobe_init(struct fprobe *fp)
> >  		fp->ops.func = fprobe_kprobe_handler;
> >  	else
> >  		fp->ops.func = fprobe_handler;
> > -	fp->ops.flags |= FTRACE_OPS_FL_SAVE_REGS;
> > +	fp->ops.flags |= FTRACE_OPS_FL_SAVE_ARGS;
> 
> so with this change you move to ftrace_caller trampoline,
> but we need ftrace_regs_caller right?

Yes, that's right.

> 
> otherwise the (!regs) check in kprobe_multi_link_handler
> will be allways true IIUC

Ah, OK. So until we move to fgraph [28/34], keep this flag SAVE_REGS
then kprobe_multi test will pass.

OK, let me keep it so.

Thank you!

> 
> jirka
> 
> >  }
> >  
> >  static int fprobe_init_rethook(struct fprobe *fp, int num)
> > diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> > index 7d2ddbcfa377..ef6b36fd05ae 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)
> >  		/*
> > 


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