Re: [RFC PATCH v3 2/4] mm/damon/core: add common code for perf event based access check

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Memory leak in `damon_perf_init()` when `perf_event_create_kernel_counter()` fails with `-ENODEV` for all CPUs.
- [Medium] Unnecessary `PERF_SAMPLE_WEIGHT_STRUCT` flag in perf event attributes arbitrarily restricts hardware compatibility.
- [Low] Linker error when `CONFIG_DAMON_VADDR=n` but `CONFIG_PERF_EVENTS=y`.
--

commit 97617fb5ae7981867a46207cd14a68c3cbb4813d
Author: Akinobu Mita <akinobu.mita@gmail.com>

mm/damon/core: add common code for perf event based access check

This commit creates the necessary infrastructure (structure definitions,
functions, sysfs interfaces) to support perf event based access checks
for both physical and virtual address space monitoring.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index a52b2962aa22a..220b105482daa 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -2933,6 +2990,7 @@ static void kdamond_init_ctx(struct damon_ctx *ctx)
>  	unsigned long sample_interval = ctx->attrs.sample_interval ?
>  		ctx->attrs.sample_interval : 1;
>  	struct damos *scheme;
> +	struct damon_perf_event *event, *next;
>  
>  	ctx->passed_sample_intervals = 0;
>  	ctx->next_aggregation_sis = ctx->attrs.aggr_interval / sample_interval;
> @@ -2945,6 +3003,15 @@ static void kdamond_init_ctx(struct damon_ctx *ctx)
>  		damos_set_next_apply_sis(scheme, ctx);
>  		damos_set_filters_default_reject(scheme);
>  	}
> +
> +	list_for_each_entry_safe(event, next, &ctx->perf_events, list) {
> +		int err = damon_perf_init(ctx, event);

If the kernel is configured with CONFIG_DAMON_VADDR=n and
CONFIG_PERF_EVENTS=y, will this cause a linker error? The function
damon_perf_init() is defined in mm/damon/vaddr.c, which would not be
compiled in that configuration.

> +
> +		if (err) {
> +			list_del(&event->list);
> +			kfree(event);
> +		}
> +	}
>  }
>  
>  /*
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index b069dbc7e3d25..1534372e7637c 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
[ ... ]
> @@ -936,6 +939,161 @@ static int damon_va_scheme_score(struct damon_ctx *context,
>  	return DAMOS_MAX_SCORE;
>  }
>  
> +#ifdef CONFIG_PERF_EVENTS
[ ... ]
> +int damon_perf_init(struct damon_ctx *ctx, struct damon_perf_event *event)
> +{
> +	struct damon_perf *perf;
> +	struct perf_event_attr attr = {
> +		.type = PERF_TYPE_RAW,
> +		.size = sizeof(attr),
> +		.type = event->attr.type,
> +		.config = event->attr.config,
> +		.config1 = event->attr.config1,
> +		.config2 = event->attr.config2,
> +		.sample_freq = event->attr.sample_freq,
> +		.freq = 1,
> +		.sample_type = PERF_SAMPLE_TIME | PERF_SAMPLE_ADDR |
> +			PERF_SAMPLE_PERIOD | PERF_SAMPLE_DATA_SRC |
> +			(event->attr.sample_phys_addr ? PERF_SAMPLE_PHYS_ADDR : 0) |
> +			PERF_SAMPLE_WEIGHT_STRUCT,

Does this configuration need to include PERF_SAMPLE_WEIGHT_STRUCT?

The overflow handler damon_perf_overflow() doesn't appear to read any
weight metrics from the sample data. If the underlying hardware PMU does
not support extended weight sampling, could this cause
perf_event_create_kernel_counter() to return -EINVAL and unnecessarily
reject the configuration?

> +		.precise_ip = 2,
> +		.pinned = 1,
> +		.disabled = 1,
> +	};
> +	int cpu;
> +	int err = -ENOMEM;
> +	bool found = false;
> +
> +	perf = kzalloc_obj(*perf);
> +	if (!perf)
> +		return -ENOMEM;
> +
> +	perf->event = alloc_percpu(typeof(*perf->event));
> +	if (!perf->event)
> +		goto free_percpu;
> +
> +	perf->buffer = alloc_percpu(typeof(*perf->buffer));
> +	if (!perf->buffer)
> +		goto free_percpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		struct perf_event *perf_event;
> +		struct damon_perf_buffer *buffer = per_cpu_ptr(perf->buffer, cpu);
> +
> +		perf_event = perf_event_create_kernel_counter(&attr, cpu, NULL,
> +				damon_perf_overflow, event);
> +		if (IS_ERR(perf_event)) {
> +			err = PTR_ERR(perf_event);
> +			if (err == -ENODEV)
> +				continue;
> +			pr_err("perf event create on CPU %d failed with %d\n", cpu, err);
> +			goto free_for_each_cpu;
> +		}
> +		found = true;
> +		*per_cpu_ptr(perf->event, cpu) = perf_event;
> +
> +		buffer->size = DAMON_PERF_INIT_RECORDS;
> +		buffer->reports = kvcalloc_node(buffer->size, sizeof(buffer->reports[0]),
> +						GFP_KERNEL, cpu_to_node(cpu));
> +		if (!buffer->reports)
> +			goto free_for_each_cpu;
> +	}
> +	event->priv = perf;
> +
> +	return found ? 0 : -ENODEV;

If perf_event_create_kernel_counter() returns -ENODEV for all possible CPUs,
the found flag remains false, and this returns -ENODEV.

Since the caller will receive a non-zero error code and free the event
wrapper without calling damon_perf_cleanup(), does this leak the memory
allocated for perf, perf->event, and perf->buffer?

> +
> +free_for_each_cpu:
> +	for_each_possible_cpu(cpu) {
> +		struct perf_event *perf_event = per_cpu(*perf->event, cpu);
> +		struct damon_perf_buffer *buffer = per_cpu_ptr(perf->buffer, cpu);
> +
> +		if (perf_event)
> +			perf_event_release_kernel(perf_event);
> +		kvfree(buffer->reports);
> +	}
> +free_percpu:
> +	free_percpu(perf->event);
> +	free_percpu(perf->buffer);
> +	kfree(perf);
> +
> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260423004211.7037-1-akinobu.mita@gmail.com?part=2