Re: [RFC PATCH v3 3/9] mm/damon: add perf-event overflow handler feeding the report ring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/damon: uninitialized memory read of data->data_src in damon_perf_overflow()
- [Medium] mm/damon: off-by-one bounds check artificially limits max probes
--

commit 96712c62a34715ea96102373f3c70dda9da07873
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>

mm/damon: add perf-event overflow handler feeding the report ring

This commit adds a PMU-agnostic perf-event overflow handler that feeds access
reports into DAMON. It runs in NMI context and translates hardware overflow
samples into physical or virtual address reports using a per-CPU ring buffer.

> diff --git a/mm/damon/perf_source.c b/mm/damon/perf_source.c
> --- /dev/null
> +++ b/mm/damon/perf_source.c

[ ... ]

> +static void damon_perf_overflow(struct perf_event *perf_event,
> +				struct perf_sample_data *data,
> +				struct pt_regs *regs)
> +{

[ ... ]

> +	if (!report.paddr && !report.vaddr) {
> +		this_cpu_inc(damon_perf_samples_filtered);
> +		return;
> +	}
> +
> +	report.is_write = !!(data->data_src.mem_op & PERF_MEM_OP_STORE);

[Severity: High]
Is it safe to unconditionally read data->data_src.mem_op here?

The perf_sample_data structure is allocated on the stack by the PMU interrupt
handler and only partially initialized:

include/linux/perf_event.h:perf_sample_data_init() {
    ...
    data->sample_flags = PERF_SAMPLE_PERIOD;
    data->period = period;
    data->dyn_size = 0;
    ...
}

If the underlying PMU does not support data source tracking (i.e.
PERF_SAMPLE_DATA_SRC is not set in data->sample_flags), data->data_src will
contain uninitialized stack garbage.

Should this read be conditionally guarded by checking if data->sample_flags &
PERF_SAMPLE_DATA_SRC is set, to avoid feeding random access reports into
DAMON?

[ ... ]

> +int damon_perf_probe_setup(struct damon_ctx *ctx,
> +			   struct damon_probe *probe,
> +			   struct damon_perf_probe_event *event)
> +{

[ ... ]

> +	/* Compute probe_idx by walking ctx->probes list */
> +	damon_for_each_probe(p, ctx) {
> +		if (p == probe)
> +			break;
> +		idx++;
> +	}
> +	/*
> +	 * Probe indices are 1-based (0 is the zero-init sentinel).
> +	 * With DAMON_MAX_PROBES slots (0..DAMON_MAX_PROBES-1), valid probe
> +	 * indices are 1..DAMON_MAX_PROBES-1, so the 0-based list position
> +	 * must be < DAMON_MAX_PROBES-1.
> +	 */
> +	if (idx >= DAMON_MAX_PROBES - 1) {
> +		err = -ENOSPC;
> +		goto release_owner;
> +	}

[Severity: Medium]
Does this bounds check artificially limit the maximum number of allowed probes
by one?

The array has DAMON_MAX_PROBES elements, meaning a 1-based probe_idx can
safely be DAMON_MAX_PROBES (which translates to array index
DAMON_MAX_PROBES - 1). 

If a user attempts to configure exactly DAMON_MAX_PROBES probes, won't this
check prematurely return -ENOSPC and drop the final slot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-damon-perf-rfc-v3-send-2026-10-03-v3-0-0f00417b41bc@gmail.com?part=3