Re: [RFT PATCH v2 3/7] perf-x86: iostat: Move iostat arch-specific implementation to util

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `iostat_prepare()` contains a Use-After-Free vulnerability if the caller's evlist already contains events.
- [High] The cross-platform iostat reporting feature is fundamentally broken and causes a NULL pointer dereference crash during `perf stat report`.
- [High] `iostat_print_metric()` uses the `die` ID as an index into `evsel->counts`, leading to an out-of-bounds read and write.
--

commit 71baa44353cfb70e43c580f7cf17077a33f1197a
Author: Yushan Wang <wangyushan12@huawei.com>

perf-x86: iostat: Move iostat arch-specific implementation to util

This commit renames the x86-specific iostat implementation and updates the
build configurations. The goal is to move the architecture-specific iostat
implementation to the utility directory to support cross-platform
recording and reporting.

> To support cross-platform iostat capabilities, like record/report on
> different platforms, architecture-specific implementation of iostat
> should be moved out of arch directory to get built. Build scripts are
> changed accordingly.

Is this cross-platform reporting capability functioning as intended?

If a user runs perf stat --iostat report, it appears iostat_prepare()
is never invoked since cmd_stat() calls __cmd_report() and returns
early. This leaves evsel->priv unpopulated.

When __cmd_report() later processes events, iostat_print_metric()
unconditionally executes:

u8 die = ((struct iio_root_port *)evsel->priv)->die;

Could dereferencing this unpopulated pointer cause a crash during reporting?

Additionally, it looks like the implementation still strictly depends on
reading the local host's sysfs topology. Does this prevent the
cross-platform capabilities from working?

[ ... ]

> diff --git a/tools/perf/arch/x86/util/iostat.c b/tools/perf/util/x86-iostat.c
> similarity index 100%
> rename from tools/perf/arch/x86/util/iostat.c
> rename to tools/perf/util/x86-iostat.c

This isn't a bug introduced by this patch, but could there be a
use-after-free in iostat_prepare() if the caller's evlist already
contains events?

If evlist->core.nr_entries > 0, it calls evlist__delete(evlist) and
reassigns the local argument parameter. Because the pointer is passed by
value, this reassignment is invisible to the caller.

Does the caller then continue to use a freed pointer, potentially causing
a crash in subsequent functions like iostat_list()?

This also isn't a bug introduced by this patch, but does
iostat_print_metric() read and write out of bounds?

It retrieves the die ID from the root port mapping, which can be greater
than 0 on multi-die systems, and passes it directly into the
perf_counts() macro as the index.

Since uncore events typically monitor exactly one CPU per die/PMU,
evsel->counts->values is allocated with a size of 1. If die is greater
than 0, does this index out of bounds when reading the value and updating
prev_count->val?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507063737.3542950-1-wangyushan12@huawei.com?part=3