On Thu, Apr 20, 2023 at 5:57 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> On Thu, 20 Apr 2023 20:08:00 +0100
> Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> > On 20/04/2023 12:26, Masami Hiramatsu (Google) wrote:
> > > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > >
> > > Support function or tracepoint parameters by name if BTF is available
> > > and the event is for function entry (This means it is available for
> > > kprobe-events, fprobe-events and tracepoint probe events.)
> > >
> > > BTF variable syntax is a bit special because it doesn't need any prefix.
> > > Also, if only the BTF variable name is given, the argument name is
> > > also becomes the BTF variable name. e.g.
> > >
> > > # echo 'p vfs_read count pos' >> dynamic_events
> > > # echo 'f vfs_write count pos' >> dynamic_events
> > > # echo 't sched_overutilized_tp rd overutilized' >> dynamic_events
> > > # cat dynamic_events
> > > p:kprobes/p_vfs_read_0 vfs_read count=count pos=pos
> > > f:fprobes/vfs_write__entry vfs_write count=count pos=pos
> > > t:tracepoints/sched_overutilized_tp sched_overutilized_tp rd=rd overutilized=overutilized
> > >
> > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > > ---
> > > kernel/trace/Kconfig | 11 ++
> > > kernel/trace/trace.c | 4 +
> > > kernel/trace/trace_fprobe.c | 49 ++++++-----
> > > kernel/trace/trace_kprobe.c | 12 +--
> > > kernel/trace/trace_probe.c | 192 +++++++++++++++++++++++++++++++++++++++++++
> > > kernel/trace/trace_probe.h | 9 ++
> > > 6 files changed, 248 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > > index 8e10a9453c96..e2b415b9fcd4 100644
> > > --- a/kernel/trace/Kconfig
> > > +++ b/kernel/trace/Kconfig
> > > @@ -664,6 +664,17 @@ config FPROBE_EVENTS
> > > and the kprobe events on function entry and exit will be
> > > transparently converted to this fprobe events.
> > >
> > > +config PROBE_EVENTS_BTF_ARGS
> > > + depends on HAVE_FUNCTION_ARG_ACCESS_API
> > > + depends on FPROBE_EVENTS || KPROBE_EVENTS
> > > + depends on DEBUG_INFO_BTF && BPF_SYSCALL
> > > + bool "Support BTF function arguments for probe events"
> > > + default y
> > > + help
> > > + The user can specify the arguments of the probe event using the names
> > > + of the arguments of the probed function. This feature only works if
> > > + the probe location is a kernel function entry or a tracepoint.
> > > +
> > > config KPROBE_EVENTS
> > > depends on KPROBES
> > > depends on HAVE_REGS_AND_STACK_ACCESS_API
> > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > > index 9da9c979faa3..0d9c48197a5c 100644
> > > --- a/kernel/trace/trace.c
> > > +++ b/kernel/trace/trace.c
> > > @@ -5670,7 +5670,11 @@ static const char readme_msg[] =
> > > "\t args: <name>=fetcharg[:type]\n"
> > > "\t fetcharg: (%<register>|$<efield>), @<address>, @<symbol>[+|-<offset>],\n"
> > > #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> > > +#ifdef CONFIG_PROBE_EVENTS_BTF_ARGS
> > > + "\t $stack<index>, $stack, $retval, $comm, $arg<N>, <argname>\n"
> > > +#else
> > > "\t $stack<index>, $stack, $retval, $comm, $arg<N>,\n"
> > > +#endif
> > > #else
> > > "\t $stack<index>, $stack, $retval, $comm,\n"
> > > #endif
> > > diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> > > index cd91bf57baac..d88079c2d2e3 100644
> > > --- a/kernel/trace/trace_fprobe.c
> > > +++ b/kernel/trace/trace_fprobe.c
> > > @@ -387,6 +387,7 @@ static void free_trace_fprobe(struct trace_fprobe *tf)
> > > static struct trace_fprobe *alloc_trace_fprobe(const char *group,
> > > const char *event,
> > > const char *symbol,
> > > + struct tracepoint *tpoint,
> > > int maxactive,
> > > int nargs, bool is_return)
> > > {
> > > @@ -406,6 +407,7 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
> > > else
> > > tf->fp.entry_handler = fentry_dispatcher;
> > >
> > > + tf->tpoint = tpoint;
> > > tf->fp.nr_maxactive = maxactive;
> > >
> > > ret = trace_probe_init(&tf->tp, event, group, false);
> > > @@ -949,8 +951,12 @@ static int __trace_fprobe_create(int argc, const char *argv[])
> > > int maxactive = 0;
> > > char buf[MAX_EVENT_NAME_LEN];
> > > char gbuf[MAX_EVENT_NAME_LEN];
> > > - unsigned int flags = TPARG_FL_KERNEL;
> > > + char sbuf[KSYM_NAME_LEN];
> > > bool is_tracepoint = false;
> > > + struct tracepoint *tpoint = NULL;
> > > + struct traceprobe_parse_context ctx = {
> > > + .flags = TPARG_FL_KERNEL | TPARG_FL_FENTRY,
> > > + };
> > >
> > > if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
> > > return -ECANCELED;
> > > @@ -1014,12 +1020,6 @@ static int __trace_fprobe_create(int argc, const char *argv[])
> > > goto parse_error;
> > > }
> > >
> > > - flags |= TPARG_FL_FENTRY;
> > > - if (is_return)
> > > - flags |= TPARG_FL_RETURN;
> > > - if (is_tracepoint)
> > > - flags |= TPARG_FL_TPOINT;
> > > -
> > > trace_probe_log_set_index(0);
> > > if (event) {
> > > ret = traceprobe_parse_event_name(&event, &group, gbuf,
> > > @@ -1031,7 +1031,8 @@ static int __trace_fprobe_create(int argc, const char *argv[])
> > > if (!event) {
> > > /* Make a new event name */
> > > if (is_tracepoint)
> > > - strscpy(buf, symbol, MAX_EVENT_NAME_LEN);
> > > + snprintf(buf, MAX_EVENT_NAME_LEN, "%s%s",
> > > + isdigit(*symbol) ? "_" : "", symbol);
> > > else
> > > snprintf(buf, MAX_EVENT_NAME_LEN, "%s__%s", symbol,
> > > is_return ? "exit" : "entry");
> > > @@ -1039,8 +1040,25 @@ static int __trace_fprobe_create(int argc, const char *argv[])
> > > event = buf;
> > > }
> > >
> > > + if (is_return)
> > > + ctx.flags |= TPARG_FL_RETURN;
> > > +
> > > + if (is_tracepoint) {
> > > + ctx.flags |= TPARG_FL_TPOINT;
> > > + tpoint = find_tracepoint(symbol);
> > > + if (!tpoint) {
> > > + trace_probe_log_set_index(1);
> > > + trace_probe_log_err(0, NO_TRACEPOINT);
> > > + goto parse_error;
> > > + }
> > > + ctx.funcname = kallsyms_lookup(
> > > + (unsigned long)tpoint->probestub,
> > > + NULL, NULL, NULL, sbuf);
> > > + } else
> > > + ctx.funcname = symbol;
> > > +
> > > /* setup a probe */
> > > - tf = alloc_trace_fprobe(group, event, symbol, maxactive,
> > > + tf = alloc_trace_fprobe(group, event, symbol, tpoint, maxactive,
> > > argc - 2, is_return);
> > > if (IS_ERR(tf)) {
> > > ret = PTR_ERR(tf);
> > > @@ -1049,24 +1067,15 @@ static int __trace_fprobe_create(int argc, const char *argv[])
> > > goto out; /* We know tf is not allocated */
> > > }
> > >
> > > - if (is_tracepoint) {
> > > - tf->tpoint = find_tracepoint(tf->symbol);
> > > - if (!tf->tpoint) {
> > > - trace_probe_log_set_index(1);
> > > - trace_probe_log_err(0, NO_TRACEPOINT);
> > > - goto parse_error;
> > > - }
> > > + if (is_tracepoint)
> > > tf->mod = __module_text_address(
> > > (unsigned long)tf->tpoint->probestub);
> > > - }
> > >
> > > argc -= 2; argv += 2;
> > > -
> > > /* parse arguments */
> > > for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
> > > - struct traceprobe_parse_context ctx = { .flags = flags };
> > > -
> > > trace_probe_log_set_index(i + 2);
> > > + ctx.offset = 0;
> > > ret = traceprobe_parse_probe_arg(&tf->tp, i, argv[i], &ctx);
> > > if (ret)
> > > goto error; /* This can be -ENOMEM */
> > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > index fd62de2a2f51..aff6c1a5e161 100644
> > > --- a/kernel/trace/trace_kprobe.c
> > > +++ b/kernel/trace/trace_kprobe.c
> > > @@ -742,7 +742,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
> > > void *addr = NULL;
> > > char buf[MAX_EVENT_NAME_LEN];
> > > char gbuf[MAX_EVENT_NAME_LEN];
> > > - unsigned int flags = TPARG_FL_KERNEL;
> > > + struct traceprobe_parse_context ctx = { .flags = TPARG_FL_KERNEL };
> > >
> > > switch (argv[0][0]) {
> > > case 'r':
> > > @@ -823,10 +823,10 @@ static int __trace_kprobe_create(int argc, const char *argv[])
> > > goto parse_error;
> > > }
> > > if (is_return)
> > > - flags |= TPARG_FL_RETURN;
> > > + ctx.flags |= TPARG_FL_RETURN;
> > > ret = kprobe_on_func_entry(NULL, symbol, offset);
> > > if (ret == 0)
> > > - flags |= TPARG_FL_FENTRY;
> > > + ctx.flags |= TPARG_FL_FENTRY;
> > > /* Defer the ENOENT case until register kprobe */
> > > if (ret == -EINVAL && is_return) {
> > > trace_probe_log_err(0, BAD_RETPROBE);
> > > @@ -856,7 +856,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
> > >
> > > /* setup a probe */
> > > tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive,
> > > - argc - 2, is_return);
> > > + argc - 2, is_return);
> > > if (IS_ERR(tk)) {
> > > ret = PTR_ERR(tk);
> > > /* This must return -ENOMEM, else there is a bug */
> > > @@ -866,10 +866,10 @@ static int __trace_kprobe_create(int argc, const char *argv[])
> > > argc -= 2; argv += 2;
> > >
> > > /* parse arguments */
> > > + ctx.funcname = symbol;
> > > for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
> > > - struct traceprobe_parse_context ctx = { .flags = flags };
> > > -
> > > trace_probe_log_set_index(i + 2);
> > > + ctx.offset = 0;
> > > ret = traceprobe_parse_probe_arg(&tk->tp, i, argv[i], &ctx);
> > > if (ret)
> > > goto error; /* This can be -ENOMEM */
> > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > > index 84a9f0446390..f55d633b3e2a 100644
> > > --- a/kernel/trace/trace_probe.c
> > > +++ b/kernel/trace/trace_probe.c
> > > @@ -300,6 +300,174 @@ static int parse_trace_event_arg(char *arg, struct fetch_insn *code,
> > > return -ENOENT;
> > > }
> > >
> > > +#ifdef CONFIG_PROBE_EVENTS_BTF_ARGS
> > > +
> > > +static DEFINE_MUTEX(tp_btf_mutex);
> > > +static struct btf *traceprobe_btf;
> > > +
> > > +static struct btf *traceprobe_get_btf(void)
> > > +{
> > > + if (!traceprobe_btf && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
> > > + mutex_lock(&tp_btf_mutex);
> > > + if (!traceprobe_btf)
> > > + traceprobe_btf = btf_parse_vmlinux();
> >
> > Apologies if I missed this in previous discussion, but should we
> > use bpf_get_btf_vmlinux() here instead, since it will
> > return an already-parsed BTF? There's a bunch of additional
> > work that btf_parse_vmlinux() does that's not needed from
> > a tracing POV.
>
> I thought bpf_get_btf_vmlinux() was only for BPF subsystem because
> it is exposed in bpf.h, not btf.h. But yeah, it is useless if we
> have 2 BTF instance. So I'll use it.
>
> By the way, I eventually would like to use the BTF things even if the
> BPF_SYSCALL is not enabled. Can we make it, or is it hard to decouple
> the BTF things from BPF?
It's hard. I'd rather keep it as-is. The test matrix is already huge.
