linux-kernel - Re: [PATCH v2 2/5] perf trace: Augment enum syscall arguments with BTF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 23 Jun 2024 19:34:19 +0800
From: Howard Chu <howardchu95@...il.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>, 
	Kan Liang <kan.liang@...ux.intel.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v2 2/5] perf trace: Augment enum syscall arguments with BTF

Hello Arnaldo,

Your changes are very very good, thank you so much.

On Sun, Jun 23, 2024 at 2:28 AM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> On Sat, Jun 22, 2024 at 12:18:10AM +0800, Howard Chu wrote:
> > Hello Arnaldo,
> >
> > I think I need to add some explanations here, for the all-in-one patch
> > that I have on the tree.
> >
> > Firstly, no use_btf is passed when constructing syscall argument
> > format. We just write is_enum when the type is enum. This is because
> > we don't load btf when we are constructing the formats, we load
> > vmlinux when a tracepoint hits.
> >
> > before:
> >
> > } else if (strstr(field->type, "enum") && use_btf != NULL) {
> > *use_btf = arg->is_enum = true;
> >
> > after:
> >
> > } else if (strstr(field->type, "enum")) {
> > arg->is_enum = true;
> >
> > Another confusing place is when we do the filtering. When a user
> > passes an enum name, say --filter="mode==HRTIMER_MODE_ABS", we do not
> > know what 'HRTIMER_MODE_ABS' means. For this case, we have to load the
> > vmlinux BTF, and match the strings to get a value, not delaying till a
> > tracepoint hits.
>
> Right, I worked on your latest series before the combined patch, and to
> solve that at filter expansion time, I did:
>
> @@ -3861,9 +3903,16 @@ static int trace__expand_filter(struct trace *trace __maybe_unused, struct evsel
>                         if (fmt->strtoul) {
>                                 u64 val;
>                                 struct syscall_arg syscall_arg = {
> -                                       .parm = fmt->parm,
> +                                       .trace = trace,
> +                                       .fmt   = fmt,
>                                 };
>
> +                               if (fmt->is_enum) {
> +                                       syscall_arg.parm = type;
> +                               } else {
> +                                       syscall_arg.parm = fmt->parm;
> +                               }
> +
>                                 if (fmt->strtoul(right, right_size, &syscall_arg, &val)) {
>
>
> And then in the enum btf strtoul we can load the btf:
>
> +static bool syscall_arg__strtoul_btf_enum(char *bf, size_t size, struct syscall_arg *arg, u64 *val)
> +{
> +       const struct btf_type *bt;
> +       char *type = arg->parm;
> +       struct btf_enum *be;
> +       struct btf *btf;
> +
> +       trace__load_vmlinux_btf(arg->trace);
> +
> +       btf = arg->trace->btf;
> +       if (btf == NULL)
> +               return false;
> +
> +       if (syscall_arg_fmt__cache_btf_enum(arg->fmt, btf, type) < 0)
> +               return false;
> +
> +       bt = arg->fmt->type;
> +       be = btf_enum(bt);
> +       for (int i = 0; i < btf_vlen(bt); ++i, ++be) {
> +               const char *name = btf__name_by_offset(btf, be->name_off);
> +               int max_len = max(size, strlen(name));
> +
> +               if (strncmp(name, bf, max_len) == 0) {
> +                       *val = be->val;
> +                       return true;
> +               }
> +       }
> +
> +       return false;
> +}
>
> And:
>
> +static int syscall_arg_fmt__cache_btf_enum(struct syscall_arg_fmt *arg_fmt, struct btf *btf, char *type)
> +{
> +       int id;
> +
> +       // Already cached?
> +       if (arg_fmt->type != NULL)
> +               return 0;
> +
> +       type = strstr(type, "enum ");
> +       if (type == NULL)
> +               return -1;
> +
> +       type += 5; // skip "enum " to get the enumeration name
> +

The shifting of enum name before doing btf__type_by_id() is handled
perfectly. And the extra wrapper function trace__btf_scnprintf() makes
a lot of sense.

Just two tiny little things:

1) the `make NO_LIBBPF=1` and `make NO_LIBELF=1` won't build

builtin-trace.c: In function ‘syscall__scnprintf_args’:
builtin-trace.c:2258:28: error: expected expression before ‘)’ token
 2258 |                            )
      |                            ^

if (val == 0 && !trace->show_zeros &&
    !(sc->arg_fmt && sc->arg_fmt[arg.idx].show_zero) &&
#ifdef HAVE_LIBBPF_SUPPORT
    !(sc->arg_fmt && sc->arg_fmt[arg.idx].strtoul == STUL_BTF_TYPE)
#endif
   )
        continue;

The fix is simple, just change the above to:

if (val == 0 && !trace->show_zeros &&
    !(sc->arg_fmt && sc->arg_fmt[arg.idx].show_zero)
#ifdef HAVE_LIBBPF_SUPPORT
    && !(sc->arg_fmt && sc->arg_fmt[arg.idx].strtoul == STUL_BTF_TYPE)
#endif
   )
        continue;

2) In this version, we won't defer the loading of vmlinux BTF till an
enum-augmentable tracepoint hits. I'm sure you made this conscious
change, not that there's anything wrong with it, just want to point it
out for clarity.

Overall, it is a great revision. Man, what can I say, this patch is
better, simpler and it handles details very well. Thank you so much
for doing this.

Thanks,
Howard