RE: [PATCH] tracing/kprobe: Fix to support kretprobe events on unloaded modules

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

> -----Original Message-----
> From: Masami Hiramatsu <mhiramat@kernel.org>
> Sent: Friday, January 29, 2021 9:45 PM
> To: Jianlin Lv <Jianlin.Lv@arm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>; Oleg Nesterov
> <oleg@redhat.com>; Ingo Molnar <mingo@redhat.com>; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] tracing/kprobe: Fix to support kretprobe events on
> unloaded modules
>
> Hi Jianlin,
>
> On Fri, 29 Jan 2021 07:45:32 +0000
> Jianlin Lv <Jianlin.Lv@arm.com> wrote:
> >
> >
> > I have verified this patch on server and it solves the kretprobe issue
> > on unloaded modules very well.
> >
> > One comments is this patch did not cover the issue I reported before,
> > https://lore.kernel.org/lkml/20210127151507.4185234-1-Jianlin.Lv@arm.c
> > om/ if echo wrong function symbol to tracefs, error_log show as :
> >
> > # echo 'r:myprobe ERROR_SYMBOL_XXX ret=%x0' >> kprobe_events # cat
> > error_log
> > [   87.746191] trace_kprobe: error: Invalid probed address or symbol
> >   Command: r:myprobe ERROR_SYMBOL_XXX ret=%x0
>
> This is the correct error message because the symbol is not exist, a bad
> symbol.
>
> Thank you,
>

Yes, it would be nice if errors could be detected before registering Kprobe.

Of course, this patch is looks good for me.

Jianlin

> >
> > Based on the current patch,  The following changes will make the error
> > log more accurate.
> >
> > @@ -828,9 +828,10 @@
> > static int trace_kprobe_create(int argc, const char *argv[]) if
> > (is_return) flags |= TPARG_FL_RETURN;
> > -    if (kprobe_on_func_entry(NULL, symbol, offset))
> > +    if (!kprobe_on_func_entry(NULL, symbol, offset))
> >         flags |= TPARG_FL_FENTRY;
> > -    if (offset && is_return && !(flags & TPARG_FL_FENTRY)) {
> > +    /* Check whether symbol is really bad or from a module */
> > +    if (!strchr(symbol, ':') && is_return && !(flags &
> > + TPARG_FL_FENTRY)) {
> >         trace_probe_log_err(0, BAD_RETPROBE);
> >         goto parse_error;
> >
> > If you don`t mind, I can send v5 of my patch after this patch be merged.
> >
> > Jianlin
> >
> >
> > 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.
>
>
> --
> 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.