Re: [PATCH 5/6] perf header: Support memory ranges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Asymmetric enforcement of `MAX_MEMORY_RANGES` allows `perf record` to create perfectly valid files that `perf report` will fatally reject.
- [Medium] TOCTOU race during sysfs parsing can cause `perf record` header generation to abort.
- [Medium] Unchecked return value of `sysfs__mountpoint()` is passed directly to `scnprintf` as a `%s` argument.
--

--- Patch [5]: [PATCH 5/6] perf header: Support memory ranges ---
commit 42917cf527bf0744d5a38079c4d626fee56bf5e9
Author: Thomas Falcon <thomas.falcon@intel.com>

perf header: Support memory ranges

Memory ranges are parsed from the ACPI MRRM table via sysfs and saved in the
perf data file under HEADER_MEMORY_RANGES. This data can be viewed with
perf-report and perf-script.

Link: https://lore.kernel.org/lkml/20250505173819.419271-1-tony.luck@intel.com/

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 091d8f7f6bd2c..5c1f767dad159 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -92,6 +92,7 @@
>  #define MAX_PMU_CAPS		512
>  #define MAX_PMU_MAPPINGS	4096
>  #define MAX_SCHED_DOMAINS	64
> +#define MAX_MEMORY_RANGES	64
>  
>  /*
>   * magic2 = "PERFILE2"
> @@ -1891,6 +1892,126 @@ static int write_cpu_domain_info(struct feat_fd *ff,
>  	return ret;
>  }
>  
> +static int memory_range__read(struct memory_range *range, int idx)
> +{
> +	char path[PATH_MAX], file[PATH_MAX];
> +	struct stat st;
> +	int tmp;
> +
> +	scnprintf(path, PATH_MAX, "firmware/acpi/memory_ranges/range%d", idx);
> +	scnprintf(file, PATH_MAX, "%s/%s", sysfs__mountpoint(), path);

[Severity: Medium]
Can sysfs__mountpoint() return NULL here if sysfs is not mounted?

If it does return NULL, would passing it directly to the %s format specifier in
scnprintf() cause a segfault on environments using strictly compliant C
libraries like musl?

> +	if (stat(file, &st))
> +		return -1;
> +

[ ... ]

> +static int memory_range__parse(struct memory_range **ranges)
> +{
> +	int i, err, nr_memory_ranges = 0;
> +	char path[PATH_MAX];
> +	struct stat st;
> +
> +	scnprintf(path, PATH_MAX, "%s/firmware/acpi/memory_ranges", sysfs__mountpoint());

[Severity: Medium]
Is there a similar risk here of sysfs__mountpoint() returning NULL when
evaluating the initial base path?

> +	if (stat(path, &st))
> +		return 0;
> +
> +	while (1) {
> +		scnprintf(path, PATH_MAX, "%s/firmware/acpi/memory_ranges/range%d", sysfs__mountpoint(), nr_memory_ranges);
> +		if (stat(path, &st))
> +			break;
> +
> +		nr_memory_ranges++;
> +	}
> +
> +	if (nr_memory_ranges == 0)
> +		return 0;
> +
> +	*ranges = zalloc(nr_memory_ranges * sizeof(struct memory_range));
> +	if (!(*ranges))
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nr_memory_ranges; i++) {
> +		struct memory_range range;
> +
> +		err = memory_range__read(&range, i);

[Severity: Medium]
Could there be a race condition if a memory range is dynamically removed from
sysfs between the initial counting loop above and this read loop?

If a range is removed, memory_range__read() will return -1, which propagates up
and aborts feature writing. Should the reader gracefully handle missing entries
that were counted moments before instead of failing the entire operation?

> +		if (err < 0)
> +			goto out_error;
> +
> +		(*ranges)[i] = range;
> +	}
> +
> +	return nr_memory_ranges;
> +
> +out_error:
> +	zfree(ranges);
> +	return -1;
> +}
> +
> +static int write_memory_ranges(struct feat_fd *ff,
> +			 struct evlist *evlist __maybe_unused)
> +{
> +	struct memory_range *ranges = NULL;
> +	int nr_memory_ranges = 0, ret;
> +
> +	nr_memory_ranges = memory_range__parse(&ranges);
> +	if (nr_memory_ranges < 0)
> +		return nr_memory_ranges;
> +
> +	ret = do_write(ff, &nr_memory_ranges, sizeof(nr_memory_ranges));
> +	if (ret < 0)
> +		goto out;
> +
> +	for (int i = 0; i < nr_memory_ranges; i++) {

[Severity: Medium]
Does this write loop allow writing more than MAX_MEMORY_RANGES (64) entries
into the perf data file if the system has more than 64 ranges?

> +		ret = do_write(ff, &ranges[i].base, sizeof(u64));
> +		if (ret < 0)
> +			goto out;

[ ... ]

> @@ -4201,6 +4339,54 @@ static int process_cpu_domain_info(struct feat_fd *ff, void *data __maybe_unused
>  	return ret;
>  }
>  
> +static int process_memory_ranges(struct feat_fd *ff, void *data __maybe_unused)
> +{
> +	struct perf_env *env = &ff->ph->env;
> +	struct memory_range *ranges, *r;
> +	u32 nr_memory_ranges, i;
> +
> +	if (do_read_u32(ff, &nr_memory_ranges))
> +		return -1;
> +
> +	if (!nr_memory_ranges) {
> +		pr_debug("memory ranges not available\n");
> +		return 0;
> +	}
> +
> +	if (nr_memory_ranges > MAX_MEMORY_RANGES) {
> +		pr_err("Invalid memory_ranges: nr_memory_ranges (%u) > %u\n",
> +		       nr_memory_ranges, MAX_MEMORY_RANGES);
> +		return -1;
> +	}

[Severity: Medium]
If the writer successfully records more than MAX_MEMORY_RANGES (64) entries on
a large system, won't this strict limit in the reader cause perfectly valid
perf.data files to be rejected?

Since the writer dynamically discovers all ranges without limits, should the
reader also process all recorded ranges instead of imposing a static 64 limit?

> +
> +	ranges = zalloc(nr_memory_ranges * sizeof(*ranges));
> +	if (!ranges)
> +		return -1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709221736.33446-1-thomas.falcon@intel.com?part=5