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

[SJ Park <sj@kernel.org>]

On Sun, 08 Nov 2026 19:30:36 +0000 sashiko-bot@kernel.org wrote:

> 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.

No.  damon_sysfs_valid_start_ctx() [1] avoids the divide-by-zero.

[1] https://lore.kernel.org/all/20260903010722.94244-2-sj@kernel.org/

> 
> [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
> 


Thanks,
SJ