Re: [PATCH v2 7/7] perf test: Add BPF JIT annotation test coverage for all disassemblers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] System-wide profiling of ambient BPF programs creates a race condition that can randomly fail the test.
- [Low] Signal interruption leaks the original perfdata file and the bpftmp directory due to temporary variable override.
--

commit 2a2752d1eb68187d4c05ebd5a52ce3cce359db69
Author: Ian Rogers <irogers@google.com>
Subject: perf test: Add BPF JIT annotation test coverage for all disassemblers

This commit expands the annotate.sh shell test to verify BPF JIT disassembly
using supported backends. It attempts to capture system BPF programs using
perf record, extracts a JITted BPF program symbol, and runs perf annotate
on it to validate temporary ELF generation.

> diff --git a/tools/perf/tests/shell/annotate.sh b/tools/perf/tests/shell/annotate.sh
> index 2c06d2670cb18..9719e592befd2 100755
> --- a/tools/perf/tests/shell/annotate.sh
> +++ b/tools/perf/tests/shell/annotate.sh

[ ... ]

> +test_bpf() {
> +  echo "Test annotate with BPF JIT output"
> +
> +  if ! perf check -q feature libbpf-strings ; then
> +    echo "BPF annotation test [Skipped - libbpf-strings not supported]"
> +    return 0
> +  fi
> +
> +  bpftmp=$(mktemp -d /tmp/__perf_test.bpf.XXXXXX)
> +  bpf_perfdata="${bpftmp}/perf.data"
> +
> +  if ! perf record -a -e cycles -F 4000 -o "${bpf_perfdata}" -- sleep 1 2> /dev/null

[Severity: Medium]
Will this probabilistic test setup cause unpredictable failures or silent skips?

By using perf record -a -- sleep 1 to capture ambient BPF programs, a race
condition occurs. If a short-lived BPF program unloads between the time the
kernel emits the KSYMBOL/BPF_EVENT and when perf record's sideband thread calls
bpf_prog_get_fd_by_id() to fetch the instructions, the JITed code won't be
saved in the perf.data file.

Later, perf report will list the BPF symbol, but perf annotate will fail
to disassemble it, causing a false test failure. Furthermore, if no ambient
BPF programs are active, the test silently skips, providing zero coverage in
most environments.

> +  then
> +    echo "BPF annotation test [Skipped - perf record -a failed, probably no privileges]"
> +    rm -rf "${bpftmp}"
> +    return 0
> +  fi

[ ... ]

> +  # temporarily override perfdata so test_bpf_disassembler uses the right file
> +  local old_perfdata="${perfdata}"
> +  perfdata="${bpf_perfdata}"
> +
> +  test_bpf_disassembler "objdump" ""
> +  test_bpf_disassembler "llvm" "libLLVM"
> +  test_bpf_disassembler "capstone" "libcapstone"
> +  test_bpf_disassembler "libasm" "libasm"
> +
> +  perfdata="${old_perfdata}"

[Severity: Low]
Is it possible for the original perfdata and bpftmp directory to be leaked
if the script is interrupted during this section?

The script uses a signal trap to run the cleanup() handler, which relies on
the global perfdata variable:

cleanup() {
  rm -rf "${perfdata}" "${perfout}"
  ...

If a signal like SIGINT occurs while perfdata is temporarily overridden,
cleanup() will only remove the inner bpf_perfdata and fail to delete the
original global perfdata file. Additionally, the new bpftmp directory is
never registered with the cleanup routine and will be left stranded in /tmp.

> +  rm -rf "${bpftmp}"
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609070732.545416-1-irogers@google.com?part=7