RE: [PATCH v3] tracing: precise log info for kretprobe addr err

[Jianlin Lv <Jianlin.Lv@arm.com>]

> -----Original Message-----
> From: Masami Hiramatsu <mhiramat@kernel.org>
> Sent: Tuesday, January 26, 2021 12:16 PM
> To: Steven Rostedt <rostedt@goodmis.org>
> Cc: Oleg Nesterov <oleg@redhat.com>; Jianlin Lv <Jianlin.Lv@arm.com>;
> mingo@redhat.com; mhiramat@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3] tracing: precise log info for kretprobe addr err
>
> On Mon, 25 Jan 2021 13:38:40 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Mon, 25 Jan 2021 19:19:27 +0100
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > On 01/26, Jianlin Lv wrote:
> > > >
> > > > When trying to create kretprobe with the wrong function symbol in
> > > > tracefs; The error is triggered in the register_trace_kprobe() and
> > > > recorded as FAIL_REG_PROBE issue,
> > > >
> > > > Example:
> > > >   $ cd /sys/kernel/debug/tracing
> > > >   $ echo 'r:myprobe ERROR_SYMBOL_XXX ret=%x0' >> kprobe_events
> > > >     bash: echo: write error: Invalid argument
> > > >   $ cat error_log
> > > >     [142797.347877] trace_kprobe: error: Failed to register probe event
> > > >     Command: r:myprobe ERROR_SYMBOL_XXX ret=%x0
> > > >                        ^
> > > >
> > > > This error can be detected in the parameter parsing stage, the
> > > > effect of applying this patch is as follows:
> > > >
> > > >   $ echo 'r:myprobe ERROR_SYMBOL_XXX ret=%x0' >> kprobe_events
> > > >     bash: echo: write error: Invalid argument
> > > >   $ cat error_log
> > > >     [415.89]trace_kprobe: error: Retprobe address must be an function
> entry
> > > >     Command: r:myprobe ERROR_SYMBOL_XXX ret=%x0
> > >
> > > IOW, the "offset != 0" check removed by this patch is obviously wrong,
> right?
> > >
>
> No, not wrong. Even offset != 0, if the symbol exists in the kernel,
> kprobe_on_func_entry() will check it.
>
> > > Agreed, but...
> > >
> > > > --- a/kernel/trace/trace_kprobe.c
> > > > +++ b/kernel/trace/trace_kprobe.c
> > > > @@ -830,7 +830,7 @@ static int trace_kprobe_create(int argc, const
> char *argv[])
> > > >  flags |= TPARG_FL_RETURN;
> > > >  if (kprobe_on_func_entry(NULL, symbol, offset))
> > > >  flags |= TPARG_FL_FENTRY;
> > > > -if (offset && is_return && !(flags & TPARG_FL_FENTRY)) {
> > > > +if (!strchr(symbol, ':') && is_return && !(flags &
> > > > +TPARG_FL_FENTRY)) {
> > >
> > > but why did you add the strchr(':') check instead?
> > >
> > > I was really puzzled until I found the this email from Masami:
> > >
> https://lore.kernel.org/lkml/20210120131406.5a992c1e434681750a0cd5d4
> > > @kernel.org/
> > >
> > > So I leave this to you and Masami, but perhaps you can document this
> > > check at least in the changelog?
> > >
> >
> > No, you are correct. That needs to be documented in the code.
>
> Agreed. There should be commented that is defered until the module is
> loaded.
>
> >
> > I was about to comment that the check requires a comment ;-)
> >
> > Jianlin,
> >
> > Care to send a v4 of the patch with a comment to why we are checking
> > the symbol for ':'.
>
> Thank you!
>
> >
> > Thanks!
> >
> > -- Steve
> >

Thanks for everyone's comments;  I will update this patch.
In addition, I have another question.

perf-probe can add a probe on a module which has not been loaded yet.
But I still get an error when I execute the following command:
# perf probe -m /lib/modules/5.11.0-rc2+/kernel/drivers/net/vxlan.ko
'vxlan_xmit%return $retval'
Failed to write event: Invalid argument
  Error: Failed to add events.

According to my investigation, __register_trace_kprobe will return -EINVAL,
because the vxlan module is not loaded;

__register_trace_kprobe
->register_kretprobe
->kprobe_on_func_entry
->return -EINVAL;

The following code snippet shows that the probe of the offline module can be
registered successfully only when the ret value is -ENOENT.

ret = __register_trace_kprobe(tk);
if (ret == -ENOENT && !trace_kprobe_module_exist(tk)) {
pr_warn("This probe might be able to register after target module is loaded. Continue.\n");
ret = 0;
}

kretprobe events not work with Offline Modules.
Is this a bug?

Jianlin

>
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.