Re: [PATCH] perf probe: Show correct error about @symbol for uprobe

Re: [PATCH] perf probe: Show correct error about @symbol for uprobe

[Masami Hiramatsu]

(To: Oleg)

On Tue, 1 Aug 2023 09:55:07 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Fri, Jul 28, 2023 at 11:19:30PM +0900, Masami Hiramatsu (Google) escreveu:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since @symbol variable access is not supported by uprobe event, it must be
> > correctly warn user instead of kernel version update.
> 
> Thanks, less cryptic now, applied.
> 
> But is that just a matter of writing code to support reading global
> variables from an uprobe?

Hm, maybe we can use "@+offset" for accessing symboled data, as far as
it is loaded with the text section.
Oleg, what do you think about accessing symbols in data section from
uprobes? Can we access it from user-land IP-relative address?

Thank you,

> 
> - Arnaldo
> 
>  
> > Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Closes: https://lore.kernel.org/all/ZLWDEjvFjrrEJODp@kernel.org/
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  tools/perf/util/probe-event.c |   13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 6e2110d605fb..c1ded85fe859 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2800,13 +2800,18 @@ static void warn_uprobe_event_compat(struct probe_trace_event *tev)
> >  	if (!tev->uprobes || tev->nargs == 0 || !buf)
> >  		goto out;
> >  
> > -	for (i = 0; i < tev->nargs; i++)
> > -		if (strglobmatch(tev->args[i].value, "[$@+-]*")) {
> > -			pr_warning("Please upgrade your kernel to at least "
> > -				   "3.14 to have access to feature %s\n",
> > +	for (i = 0; i < tev->nargs; i++) {
> > +		if (strchr(tev->args[i].value, '@')) {
> > +			pr_warning("%s accesses a variable by symbol name, but that is not supported for user application probe.\n",
> > +				   tev->args[i].value);
> > +			break;
> > +		}
> > +		if (strglobmatch(tev->args[i].value, "[$+-]*")) {
> > +			pr_warning("Please upgrade your kernel to at least 3.14 to have access to feature %s\n",
> >  				   tev->args[i].value);
> >  			break;
> >  		}
> > +	}
> >  out:
> >  	free(buf);
> >  }
> > 
> 
> -- 
> 
> - Arnaldo


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

Re: [PATCH] perf probe: Show correct error about @symbol for uprobe

[Oleg Nesterov]

Hi Masami,

On 08/01, Masami Hiramatsu wrote:
>
> Oleg, what do you think about accessing symbols in data section from
> uprobes? Can we access it from user-land IP-relative address?

Sorry, I don't I understand... I don't even understand the context, can't
find the whole thread on lore.kernel.org/lkml. Plus I forgot EVERYTHING
I knew about tracing/uprobes anyway ;)

but when I look at traceprobe_parse_probe_arg() paths it seems to me that
uprobe can fetch the IP-relative address, not sure about the syntax but
perhaps something like xxx=+OFFSET(%ip). The question is how tools/perf
can calculate this OFFSET. But I am sure you understand this all much
better than me.

> > > --- a/tools/perf/util/probe-event.c
> > > +++ b/tools/perf/util/probe-event.c
> > > @@ -2800,13 +2800,18 @@ static void warn_uprobe_event_compat(struct probe_trace_event *tev)
> > >  	if (!tev->uprobes || tev->nargs == 0 || !buf)
> > >  		goto out;
> > >
> > > -	for (i = 0; i < tev->nargs; i++)
> > > -		if (strglobmatch(tev->args[i].value, "[$@+-]*")) {
> > > -			pr_warning("Please upgrade your kernel to at least "
> > > -				   "3.14 to have access to feature %s\n",
> > > +	for (i = 0; i < tev->nargs; i++) {
> > > +		if (strchr(tev->args[i].value, '@')) {
> > > +			pr_warning("%s accesses a variable by symbol name, but that is not supported for user application probe.\n",
> > > +				   tev->args[i].value);
> > > +			break;

IIUC without this change @symbol will trigger the

			/* uprobes don't support symbols */
			if (!(ctx->flags & TPARG_FL_KERNEL)) {
				trace_probe_log_err(ctx->offset, SYM_ON_UPROBE);
				return -EINVAL;
			}

in parse_probe_arg(), right?

So FWIW the patch looks fine to me, but as you have mentioned tools/perf
could probably (try to) turn @symbol into @+symbol_offset_in_file...

In short, sorry for spam, I can't help ;)

And just in case, I am on PTO till Aug 14, won't be able to read emails
till then.

Oleg.

Re: [PATCH] perf probe: Show correct error about @symbol for uprobe

[Masami Hiramatsu]

On Thu, 3 Aug 2023 19:31:06 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> Hi Masami,
> 
> On 08/01, Masami Hiramatsu wrote:
> >
> > Oleg, what do you think about accessing symbols in data section from
> > uprobes? Can we access it from user-land IP-relative address?
> 
> Sorry, I don't I understand... I don't even understand the context, can't
> find the whole thread on lore.kernel.org/lkml. Plus I forgot EVERYTHING
> I knew about tracing/uprobes anyway ;)

Oh, but thank you for replying :)

> 
> but when I look at traceprobe_parse_probe_arg() paths it seems to me that
> uprobe can fetch the IP-relative address, not sure about the syntax but
> perhaps something like xxx=+OFFSET(%ip). The question is how tools/perf
> can calculate this OFFSET. But I am sure you understand this all much
> better than me.

Yes, var=@+offset is the syntax. That will access the data in same file
(and maybe in the same section?)

> 
> > > > --- a/tools/perf/util/probe-event.c
> > > > +++ b/tools/perf/util/probe-event.c
> > > > @@ -2800,13 +2800,18 @@ static void warn_uprobe_event_compat(struct probe_trace_event *tev)
> > > >  	if (!tev->uprobes || tev->nargs == 0 || !buf)
> > > >  		goto out;
> > > >
> > > > -	for (i = 0; i < tev->nargs; i++)
> > > > -		if (strglobmatch(tev->args[i].value, "[$@+-]*")) {
> > > > -			pr_warning("Please upgrade your kernel to at least "
> > > > -				   "3.14 to have access to feature %s\n",
> > > > +	for (i = 0; i < tev->nargs; i++) {
> > > > +		if (strchr(tev->args[i].value, '@')) {
> > > > +			pr_warning("%s accesses a variable by symbol name, but that is not supported for user application probe.\n",
> > > > +				   tev->args[i].value);
> > > > +			break;
> 
> IIUC without this change @symbol will trigger the
> 
> 			/* uprobes don't support symbols */
> 			if (!(ctx->flags & TPARG_FL_KERNEL)) {
> 				trace_probe_log_err(ctx->offset, SYM_ON_UPROBE);
> 				return -EINVAL;
> 			}
> 
> in parse_probe_arg(), right?

Yes, that's right.

> 
> So FWIW the patch looks fine to me, but as you have mentioned tools/perf
> could probably (try to) turn @symbol into @+symbol_offset_in_file...

Yeah, that's a good point. Maybe I should try to find the data symbol
and find the offset. One thing I'm not sure is the address in the data section
maybe different from the address in the code section, and @+addr seems to
point the data in the code section because it calculate the offset from
the ip address.

> 
> In short, sorry for spam, I can't help ;)

No problem and thanks for your reply.

> 
> And just in case, I am on PTO till Aug 14, won't be able to read emails
> till then.
> 
> Oleg.
> 

Thank you!

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