Re: [PATCH 6.18.y 3/5] mm/damon/sysfs-schemes: read sysfs_filter->sz_range only once

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mm/damon/sysfs: TOCTOU race in damos_sysfs_add_quota_score()
  [1]
--

commit 244e62bfada7cc56b0025da40ea03dad6d9c6420
Author: SJ Park <sj@kernel.org>

mm/damon/sysfs-schemes: read sysfs_filter->sz_range only once

This commit copies sysfs_filter->sz_range into filter->sz_range before
validating it in damon_sysfs_add_scheme_filters() so that a concurrent
sysfs update cannot pass an unvalidated size range to the core layer.

Link: https://lore.kernel.org/20260914142327.92510-3-sj@kernel.org

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 37e0a8bef4b1f..0f945ef043709 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -2493,12 +2493,11 @@ static int damon_sysfs_add_scheme_filters(struct damos *scheme,
>  		} else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
>  			filter->target_idx = sysfs_filter->target_idx;
>  		} else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE_SIZE) {
> -			if (sysfs_filter->sz_range.min >
> -					sysfs_filter->sz_range.max) {
> +			filter->sz_range = sysfs_filter->sz_range;
> +			if (filter->sz_range.min > filter->sz_range.max) {
>  				damos_destroy_filter(filter);
>  				return -EINVAL;
>  			}
> -			filter->sz_range = sysfs_filter->sz_range;
>  		}
>  
>  		damos_add_filter(scheme, filter);

[Severity: High]
This problem wasn't introduced by this patch, but can
damos_sysfs_add_quota_score() just below hit a similar double-read race on
sysfs_goal->target_value [1]?

mm/damon/sysfs-schemes.c:damos_sysfs_add_quota_score() {
	...
		if (!sysfs_goal->target_value)
			continue;

		goal = damos_new_quota_goal(sysfs_goal->metric,
				sysfs_goal->target_value);
	...
}

target_value_store() updates sysfs_goal->target_value without holding
damon_sysfs_lock:

CPU0                              CPU1
damos_sysfs_add_quota_score()
  if (!sysfs_goal->target_value)
                                  target_value_store()
                                    kstrtoul(..., &goal->target_value)
  damos_new_quota_goal(...,
      sysfs_goal->target_value)
  ...
damos_quota_score()
  mult_frac(...,
      goal->target_value)  <-- divide-by-zero

If a concurrent write sets sysfs_goal->target_value to 0 between the check
and the second read, damos_new_quota_goal() creates a goal with a zero
target_value, which later causes a divide-by-zero in damos_quota_score().
sysfs_goal->metric is also read twice in damos_sysfs_add_quota_score(), so
a concurrent metric_store() between damos_new_quota_goal() and the switch
can leave goal->nid or goal->current_value uninitialized.

[1] https://sashiko.dev/#/bug/linux-7f814c9a-986f-4103-a55a-ef9537dcf62b

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