Re: [PATCH v1 03/41] perf pmu: Add from_env parameter to perf_pmu__init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf pmu: evsel__is_drm() and evsel__is_hwmon() bypass open/read on from_env
--

commit ba6ab0f45b9bb32deca7319cf88953bc1e7ecb4b
Author: Ian Rogers <irogers@google.com>

perf pmu: Add from_env parameter to perf_pmu__init()

This commit adds a from_env boolean field to struct perf_pmu initialized via
perf_pmu__init() to distinguish PMUs reconstructed from perf_env headers from
live host PMUs, and guards tp_pmu, hwmon_pmu, and drm_pmu callbacks on
!pmu->from_env.

> diff --git a/tools/perf/util/drm_pmu.c b/tools/perf/util/drm_pmu.c
> index b8badae7015c6..cbbd5aafd24b8 100644
> --- a/tools/perf/util/drm_pmu.c
> +++ b/tools/perf/util/drm_pmu.c
> @@ -68,7 +68,7 @@ bool perf_pmu__is_drm(const struct perf_pmu *pmu)
>  
>  bool evsel__is_drm(const struct evsel *evsel)
>  {
> -	return perf_pmu__is_drm(evsel->pmu);
> +	return perf_pmu__is_drm(evsel->pmu) && !evsel->pmu->from_env;
>  }

[ ... ]

> @@ -658,11 +658,17 @@ static uint64_t drm_pmu__read_for_pid(struct drm_pmu *drm, struct evsel *evsel,
>  
>  int evsel__drm_pmu_read(struct evsel *evsel, int cpu_map_idx, int thread)
>  {
> -	struct drm_pmu *drm = container_of(evsel->pmu, struct drm_pmu, pmu);
> +	struct drm_pmu *drm;
>  	struct perf_counts_values *count, *old_count = NULL;
> -	int pid = perf_thread_map__pid(evsel->core.threads, thread);
> +	int pid;
>  	uint64_t counter;
>  
> +	if (!evsel->pmu || evsel->pmu->from_env)
> +		return -EINVAL;
> +
> +	drm = container_of(evsel->pmu, struct drm_pmu, pmu);
> +	pid = perf_thread_map__pid(evsel->core.threads, thread);
> +
>  	if (pid != -1)
>  		counter = drm_pmu__read_for_pid(drm, evsel, pid);
>  	else
> diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
> index 96aeb379e169d..fab9f09d0253d 100644
> --- a/tools/perf/util/hwmon_pmu.c
> +++ b/tools/perf/util/hwmon_pmu.c
> @@ -132,7 +132,7 @@ bool perf_pmu__is_hwmon(const struct perf_pmu *pmu)
>  
>  bool evsel__is_hwmon(const struct evsel *evsel)
>  {
> -	return perf_pmu__is_hwmon(evsel->pmu);
> +	return perf_pmu__is_hwmon(evsel->pmu) && !evsel->pmu->from_env;
>  }

[ ... ]

> @@ -763,12 +763,18 @@ int evsel__hwmon_pmu_open(struct evsel *evsel,
>  			  struct perf_thread_map *threads,
>  			  int start_cpu_map_idx, int end_cpu_map_idx)
>  {
> -	struct hwmon_pmu *hwm = container_of(evsel->pmu, struct hwmon_pmu, pmu);
> +	struct hwmon_pmu *hwm;
>  	union hwmon_pmu_event_key key = {
>  		.type_and_num = evsel->core.attr.config,
>  	};
>  	int idx = 0, thread = 0, nthreads, err = 0;
> -	int dir = open(hwm->hwmon_dir, O_CLOEXEC | O_DIRECTORY | O_RDONLY);
> +	int dir;
> +
> +	if (!evsel->pmu || evsel->pmu->from_env)
> +		return -EINVAL;

[Severity: Medium]
Should evsel__is_drm() in tools/perf/util/drm_pmu.c and evsel__is_hwmon() in
tools/perf/util/hwmon_pmu.c really return false when evsel->pmu->from_env is
true?

In tools/perf/util/evsel.c, evsel__is_hwmon() and evsel__is_drm() are used by
evsel__open_cpu() and evsel__read_counter() to dispatch to the DRM and hwmon
open/read callbacks instead of the standard perf_event_open() and fd read
paths:

tools/perf/util/evsel.c:evsel__open_cpu() {
	if (evsel__is_hwmon(evsel)) {
		err = evsel__hwmon_pmu_open(evsel, threads,
					    start_cpu_map_idx,
					    end_cpu_map_idx);
		goto out;
	}
	if (evsel__is_drm(evsel)) {
		err = evsel__drm_pmu_open(evsel, threads,
					  start_cpu_map_idx,
					  end_cpu_map_idx);
		goto out;
	}
}

tools/perf/util/evsel.c:evsel__read_counter() {
	if (evsel__is_hwmon(evsel))
		return evsel__hwmon_pmu_read(evsel, cpu_map_idx, thread);

	if (evsel__is_drm(evsel))
		return evsel__drm_pmu_read(evsel, cpu_map_idx, thread);
}

Because evsel__is_drm() and evsel__is_hwmon() now return false when
evsel->pmu->from_env is true, evsel__open_cpu() skips evsel__hwmon_pmu_open()
and evsel__drm_pmu_open() and falls through into the sys_perf_event_open()
loop with synthetic userspace PMU types (PERF_PMU_TYPE_DRM_START..END or
PERF_PMU_TYPE_HWMON_START..END), and evsel__read_counter() skips
evsel__hwmon_pmu_read() and evsel__drm_pmu_read() and falls through to
evsel__read_group() / evsel__read_one().

This also makes the new evsel->pmu->from_env checks in evsel__drm_pmu_read()
and evsel__hwmon_pmu_open() unreachable from evsel.c, and leaves
evsel__is_drm() and evsel__is_hwmon() inconsistent with perf_pmu__is_drm(),
perf_pmu__is_hwmon(), and evsel__is_non_perf_event_open_pmu().

Would it be better to keep evsel__is_drm() and evsel__is_hwmon() checking
only perf_pmu__is_drm(evsel->pmu) and perf_pmu__is_hwmon(evsel->pmu) so that
evsel__open_cpu() and evsel__read_counter() dispatch to the DRM/hwmon
callbacks and return -EINVAL on from_env?

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