Re: [RFC PATCH 1/8] mm/damon/sysfs-schemes: read sysfs_filter->addr_range only once

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A double-read of `sysfs_goal->metric` in `damos_sysfs_add_quota_score()` causes uninitialized memory use in the core DAMOS quota auto-tuning logic.
--

commit 4b90a6724a684c24da0cc60a260ec280b6d479ac
Author: SJ Park <sj@kernel.org>
Subject: mm/damon/sysfs-schemes: read sysfs_filter->addr_range only once

This commit fixes a potential race in the DAMON sysfs interface where
sysfs_filter->addr_range is read twice. By copying the address range first
and validating the copy, it prevents invalid configurations from reaching
the core layer.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 3de4d804e049f..3c1c1cb387fec 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -2831,12 +2831,12 @@ static int damon_sysfs_add_scheme_filters(struct damos *scheme,
>  				return err;
>  			}
>  		} else if (filter->type == DAMOS_FILTER_TYPE_ADDR) {
> -			if (sysfs_filter->addr_range.end <
> -					sysfs_filter->addr_range.start) {
> +			filter->addr_range = sysfs_filter->addr_range;
> +			if (filter->addr_range.end <
> +					filter->addr_range.start) {
>  				damos_destroy_filter(filter);
>  				return -EINVAL;
>  			}
> -			filter->addr_range = sysfs_filter->addr_range;
>  		} else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
>  			filter->target_idx = sysfs_filter->target_idx;
>  		} else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE_SIZE) {

[ ... ]

While reviewing this fix for the double-read of addr_range, I noticed a
similar pattern in the same file:

mm/damon/sysfs-schemes.c:damos_sysfs_add_quota_score() {
    ...
		goal = damos_new_quota_goal(sysfs_goal->metric,
				sysfs_goal->target_value);
		if (!goal)
			return -ENOMEM;
		switch (sysfs_goal->metric) {
		case DAMOS_QUOTA_USER_INPUT:
			goal->current_value = sysfs_goal->current_value;
			break;
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code double-read
sysfs_goal->metric?

The sysfs_goal->metric value is read first to allocate and initialize
goal->metric via damos_new_quota_goal(), and then read a second time
in the switch statement.

If the metric changes concurrently from DAMOS_QUOTA_USER_INPUT to another
value between these two reads, could the switch statement branch away and
skip the initialization of goal->current_value?

Since kmalloc does not zero memory, wouldn't goal->current_value be left
uninitialized, causing the core auto-tuning logic to read garbage data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912191807.138502-1-sj@kernel.org?part=1