linux-kernel - Re: [PATCH 3/3] perf: collect BPF metadata from new programs, and display the new event

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fXkfVb4gwuSXr_yZMj8ctPr8LHs-Js7g9hP46dhkU_kQQ@mail.gmail.com>
Date: Thu, 29 May 2025 16:27:17 -0700
From: Ian Rogers <irogers@...gle.com>
To: Blake Jones <blakejones@...gle.com>
Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, 
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	Chun-Tse Shao <ctshao@...gle.com>, Zhongqiu Han <quic_zhonhan@...cinc.com>, 
	James Clark <james.clark@...aro.org>, Charlie Jenkins <charlie@...osinc.com>, 
	Andi Kleen <ak@...ux.intel.com>, Dmitry Vyukov <dvyukov@...gle.com>, Leo Yan <leo.yan@....com>, 
	Yujie Liu <yujie.liu@...el.com>, Graham Woodward <graham.woodward@....com>, 
	Yicong Yang <yangyicong@...ilicon.com>, Ben Gainey <ben.gainey@....com>, 
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org, 
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 3/3] perf: collect BPF metadata from new programs, and
 display the new event

On Thu, May 29, 2025 at 4:09 PM Blake Jones <blakejones@...gle.com> wrote:
>
> Hi Ian,
>
> Thanks for your comments!
>
> On Thu, May 29, 2025 at 11:12 AM Ian Rogers <irogers@...gle.com> wrote:
> > On Wed, May 21, 2025 at 3:27 PM Blake Jones <blakejones@...gle.com> wrote:
> > > diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/util/bpf_skel/sample_filter.bpf.c
> > > [...]
> > > +// This is used by tests/shell/record_bpf_metadata.sh
> > > +// to verify that BPF metadata generation works.
> > > +const int bpf_metadata_test_value SEC(".rodata") = 42;
> >
> > This is a bit random.
>
> Yeah, that's fair. I added it because it was a straightforward way to reliably
> get a known value that I could observe from tests/shell/test_bpf_metadata.sh.
>
> > For the non-BPF C code we have a build generated
> > PERF-VERSION-FILE that contains something like `#define PERF_VERSION
> > "6.15.rc7.ge450e74276d2"`. I wonder having something like
> ]> [...]
> > would be more useful/meaningful. Perhaps the build could inject the
> > variable to avoid duplicating it all the BPF skeletons.
>
> I could do this if you'd like. It would make it harder for my test to check
> that it was reporting the right value, because the PERF-VERSION-FILE defines
> PERF_VERSION in a way that's useful for C programs but not shell scripts.
> I'd just have the test check that it was reporting some string (maybe one
> with at least one dot in it, if I can stably make that assumption). WDYT?

It should be okay as you can compare the string against that reported
by `perf version`. On my build in `/tmp/perf`:
```
$ /tmp/perf/perf version
perf version 6.15.rc7.gb9ac06abfde9
$ cat /tmp/perf/PERF-VERSION-FILE
#define PERF_VERSION "6.15.rc7.gb9ac06abfde9"
```

> > nit: I wonder for testing it would be interesting to have 0 and >1
> > metadata values tested too. We may want to have test programs
> > explicitly for that, in tools/perf/tests.
>
> My testing right now depends pretty heavily on the fact that perf will load
> sample_filter.bpf.o for me; this seems much better than trying to load a
> BPF program manually from a test.
>
> If I could find other perf invocations that would load other BPF programs
> automatically, I could potentially test the "0 metadata" and ">1 metadata"
> cases. That would involve more BPF-program-specific modifications, though,
> which would be closer to the thing you objected to above. So to me this
> doesn't seem worth the effort.

Ok, and the main 1 metadata case is well covered by the test you added.

Thanks,
Ian