Re: [RFT PATCH v2 6/7] perf-iostat: Make x86 iostat compatible with new iostat framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The global constructor `x86_iio_iostat_pmu_init` unconditionally triggers a synchronous sysfs scan, severely regressing binary startup performance.
- [High] Reassigning the local `evlist` pointer in `iio_iostat_prepare` causes a Use-After-Free in the caller.
- [High] Failure in `realloc` leaves `nr_entries` incorrectly incremented, leading to an out-of-bounds read and invalid free.
- [Medium] The PMU matching probe `iio_iostat_probe` violates subsystem guidelines by lacking a `fake_pmu` fallback.
--

commit 315073cef675058ba43972361532dbecbed38e8d
Author: Shiju Jose <shiju.jose@huawei.com>

perf-iostat: Make x86 iostat compatible with new iostat framework
    
Change the original x86 iio iostat supporter to be compatible with the
set of iostat frameworks. The matching function of x86 iio may not be
correct.

> diff --git a/tools/perf/util/x86-iostat.c b/tools/perf/util/x86-iostat.c
> index f8d4c97185949..778655cbc2a0a 100644
> --- a/tools/perf/util/x86-iostat.c
> +++ b/tools/perf/util/x86-iostat.c
> @@ -354,7 +354,7 @@ static void iostat_prefix(struct evlist *evlist,
>  	}
>  }
>  
> -int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config)
> +static int iio_iostat_prepare(struct evlist *evlist, struct perf_stat_config *config)
>  {
>  	if (evlist->core.nr_entries > 0) {
>  		pr_warning("The -e and -M options are not supported."
>  			   "All chosen events/metrics will be dropped\n");
>  		evlist__delete(evlist);
>  		evlist = evlist__new();

This problem wasn't introduced by this patch, but does reassigning the local
evlist parameter here leave the caller with a dangling pointer?

When evlist__delete(evlist) frees the memory, the local evlist pointer is
updated with evlist__new(), but the caller's evsel_list pointer remains
unchanged.

Could this cause a use-after-free when the caller subsequently passes its
pointer to functions like iostat_list()?

[ ... ]

> +/*
> + * FIXME: pmu name prefix match might not work for x86 iio.
> + */
> +static bool iio_iostat_probe(struct iostat_pmu *iostat_pmu)
> +{
> +	return perf_pmus__scan_matching_wildcard(NULL, iostat_pmu->pmu_name_wildcard);
> +}

Does this probe function need to handle the fake_pmu fallback behavior?

According to the perf subsystem guidelines, any code performing PMU lookups
by name must provide a fallback when fake_pmu is enabled.

Since this function does not handle it and evaluates at constructor time
without a parse_state, does it return NULL on non-Intel architectures and
refuse to register the x86 iostat PMU, breaking cross-architecture tests?

> +
> +static struct iostat_pmu x86_iio_iostat_pmu_list[]  = {
> +	{
> +		.pmu_name_wildcard = "uncore_iio*",
> +		.match = iio_iostat_probe,
> +		.prepare = iio_iostat_prepare,
> +		.parse = iio_iostat_parse,
> +		.list = iio_iostat_list,
> +		.print_header_prefix = iio_iostat_print_header_prefix,
> +		.print_metric = iio_iostat_print_metric,
> +		.print_counters = iio_iostat_print_counters,
> +		.release = iio_iostat_release,
> +	},
> +};
> +
> +static void __attribute__((constructor)) x86_iio_iostat_pmu_init(void)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(x86_iio_iostat_pmu_list); i++)
> +		register_iostat_pmu(&x86_iio_iostat_pmu_list[i]);
> +}

Does executing this logic in a global constructor cause a binary startup
regression?

This initialization calls register_iostat_pmu(), which invokes
iio_iostat_probe(), which then calls perf_pmus__scan_matching_wildcard().

This unconditionally executes pmu_read_sysfs() to open and parse the
/sys/bus/event_source/devices directory.

Does this force heavy file I/O on every single execution of the perf binary
(such as "perf help"), preventing lazy initialization?

Additionally, this problem wasn't introduced by this patch, but looking at
iio_root_ports_list_insert() in tools/perf/util/x86-iostat.c:

static int iio_root_ports_list_insert(struct iio_root_ports_list *list,
				      struct iio_root_port * const rp)
{
...
	if (list && rp) {
		rp->idx = list->nr_entries++;
		tmp_buf = realloc(list->rps,
				  list->nr_entries * sizeof(*list->rps));
		if (!tmp_buf) {
			pr_err("Failed to realloc memory\n");
			return -ENOMEM;
		}
...

If realloc() fails, does this leave list->nr_entries incorrectly incremented?

When the error path subsequently calls iio_root_ports_list_free(), does it
iterate up to the inflated nr_entries and cause an out-of-bounds array read of
list->rps, followed by an invalid zfree()?

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