Re: [RFC PATCH v3 6/9] mm/damon/sysfs: expose perf_event prep attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: damon: unconditionally accepting DAMON_PREP_PERF_EVENT in sysfs
- [Low] mm: damon: missing lock in sysfs stores for perf config causes TOCTOU
--

commit 78f93348dd1c6285a193d16bcc3c5698cbede00f
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>

mm/damon/sysfs: expose perf_event prep attributes

This commit wires the DAMON_PREP_PERF_EVENT prep type into sysfs, allowing
user space to configure a PMU hotness probe through the preps/N/ directory.
It adds various perf_event attributes and sets up the probe descriptor when
the kdamond is turned on.

> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 80e6fc8004e5d..36bccf9480254 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
[ ... ]
> @@ -834,8 +854,122 @@ static void damon_sysfs_prep_release(struct kobject *kobj)
>  static struct kobj_attribute damon_sysfs_prep_prep_action_attr =
>  		__ATTR_RW_MODE(prep_action, 0600);
>  
> +/*
> + * perf_event configuration attributes.  These mirror a subset of
> + * perf_event_attr and are only meaningful when prep_action is "perf_event".
> + * They select the PMU (via type/config) and its sampling parameters, and are
> + * copied into the perf-event probe when the kdamond is turned on.
> + *
> + * The sysfs file names stay bare (type, config, ...) while the backing C
> + * symbols are prefixed to avoid clashing with identically named attributes
> + * elsewhere in this file.
> + */
[ ... ]
> +#define DAMON_SYSFS_PREP_PERF_U64(name, field)				\
> +static ssize_t damon_sysfs_prep_##name##_show(struct kobject *kobj,	\
> +		struct kobj_attribute *attr, char *buf)			\
> +{									\
> +	struct damon_sysfs_prep *prep = container_of(kobj,		\
> +			struct damon_sysfs_prep, kobj);			\
> +	return sysfs_emit(buf, "%llu\n", prep->field);			\
> +}									\
> +static ssize_t damon_sysfs_prep_##name##_store(struct kobject *kobj,	\
> +		struct kobj_attribute *attr, const char *buf,		\
> +		size_t count)						\
> +{									\
> +	struct damon_sysfs_prep *prep = container_of(kobj,		\
> +			struct damon_sysfs_prep, kobj);			\
> +	u64 v;								\
> +	int err = kstrtou64(buf, 0, &v);				\
> +	if (err)							\
> +		return err;						\
> +	prep->field = v;						\
> +	return count;							\
> +}									\
> +static struct kobj_attribute damon_sysfs_prep_##name##_attr = __ATTR(name, \
> +		0600, damon_sysfs_prep_##name##_show,			\
> +		damon_sysfs_prep_##name##_store)

[Severity: Low]
Does this code allow concurrent sysfs writes to bypass the mutual exclusivity
checks? The generated store functions permit concurrent modification of the prep
configuration fields since they do not take damon_sysfs_lock.

[ ... ]
> @@ -2261,14 +2395,62 @@ static int damon_sysfs_set_preps(struct damon_probe *probe,
>  		struct damon_sysfs_preps *sys_preps)
>  {
>  	int i;
> +	bool seen_perf_prep = false;
>  
>  	for (i = 0; i < sys_preps->nr; i++) {
>  		struct damon_sysfs_prep *sys_prep = sys_preps->preps_arr[i];
>  		struct damon_prep *prep;
>  
> +		/*
> +		 * period and freq are mutually exclusive perf sampling modes;
> +		 * reject a config that sets both before it can arm a counter.
> +		 */
> +		if (sys_prep->action == DAMON_PREP_PERF_EVENT &&

[Severity: High]
Is it possible for DAMON_PREP_PERF_EVENT to be parsed and accepted
unconditionally here, even when the kernel feature CONFIG_DAMON_PERF_SOURCE is
disabled?

> +		    sys_prep->sample_period && sys_prep->sample_freq)
> +			return -EINVAL;

[Severity: Low]
Following up on the sysfs locking question above, could the shared configuration
variables be evaluated multiple times without READ_ONCE() or local copies?
This could leave a window where their values change between this check and
the assignment below, potentially invalidating the check and passing an invalid
perf configuration to the core.

[ ... ]
> @@ -2322,13 +2567,21 @@ static int damon_sysfs_set_probe(struct damon_probe *probe,
>  			return err;
>  	}
>  	sys_filters = sys_probe->filters;
> -	if (!sys_filters)
> -		return 0;
> -	return damon_sysfs_set_filters(probe, sys_filters);
> +	if (sys_filters) {
> +		err = damon_sysfs_set_filters(probe, sys_filters);
> +		if (err)
> +			return err;
> +	}
> +#ifdef CONFIG_DAMON_PERF_SOURCE
> +	err = damon_sysfs_set_perf_probe(ctx, probe, arm);
> +	if (err)
> +		return err;
> +#endif
> +	return 0;
>  }

[Severity: High]
When CONFIG_DAMON_PERF_SOURCE is disabled, does this silently accept the
perf configuration without setting probe->event_driven = true?

If so, it seems the probe might fall back to broken software sampling without
clearing the accessed bit, which could cause the probe to hallucinate 100%
memory hotness forever and potentially evict all hot pages.

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