Re: [PATCH v3] trace/kprobe: Display the actual notrace function when rejecting a probe

[Naveen N Rao]

On Thu, Dec 14, 2023 at 08:02:10AM +0900, Masami Hiramatsu wrote:
> On Wed, 13 Dec 2023 20:09:14 +0530
> Naveen N Rao <naveen@xxxxxxxxxx> wrote:
> 
> > Trying to probe update_sd_lb_stats() using perf results in the below
> > message in the kernel log:
> >   trace_kprobe: Could not probe notrace function _text
> > 
> > This is because 'perf probe' specifies the kprobe location as an offset
> > from '_text':
> >   $ sudo perf probe -D update_sd_lb_stats
> >   p:probe/update_sd_lb_stats _text+1830728
> > 
> > However, the error message is misleading and doesn't help convey the
> > actual notrace function that is being probed. Fix this by looking up the
> > actual function name that is being probed. With this fix, we now get the
> > below message in the kernel log:
> >   trace_kprobe: Could not probe notrace function update_sd_lb_stats.constprop.0
> > 
> > Signed-off-by: Naveen N Rao <naveen@xxxxxxxxxx>
> > ---
> > v3: Remove tk parameter from within_notrace_func() as suggested by 
> > Masami
> > 
> >  kernel/trace/trace_kprobe.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 3d7a180a8427..dc36c6ed6131 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -449,9 +449,8 @@ static bool __within_notrace_func(unsigned long addr)
> >  	return !ftrace_location_range(addr, addr + size - 1);
> >  }
> >  
> > -static bool within_notrace_func(struct trace_kprobe *tk)
> > +static bool within_notrace_func(unsigned long addr)
> >  {
> > -	unsigned long addr = trace_kprobe_address(tk);
> >  	char symname[KSYM_NAME_LEN], *p;
> >  
> >  	if (!__within_notrace_func(addr))
> > @@ -471,12 +470,14 @@ static bool within_notrace_func(struct trace_kprobe *tk)
> >  	return true;
> >  }
> >  #else
> > -#define within_notrace_func(tk)	(false)
> > +#define within_notrace_func(addr)	(false)
> >  #endif
> >  
> >  /* Internal register function - just handle k*probes and flags */
> >  static int __register_trace_kprobe(struct trace_kprobe *tk)
> >  {
> > +	unsigned long addr = trace_kprobe_address(tk);
> > +	char symname[KSYM_NAME_LEN];
> >  	int i, ret;
> >  
> >  	ret = security_locked_down(LOCKDOWN_KPROBES);
> > @@ -486,9 +487,9 @@ static int __register_trace_kprobe(struct trace_kprobe *tk)
> >  	if (trace_kprobe_is_registered(tk))
> >  		return -EINVAL;
> >  
> > -	if (within_notrace_func(tk)) {
> > +	if (within_notrace_func(addr)) {
> >  		pr_warn("Could not probe notrace function %s\n",
> > -			trace_kprobe_symbol(tk));
> > +			lookup_symbol_name(addr, symname) ? trace_kprobe_symbol(tk) : symname);
> 
> Can we just use %ps and (void *)trace_kprobe_address(tk) here?
> 
> That will be simpler.

Indeed - that is much simpler. v4 on its way...

Thanks!
- Naveen