Re: [RFC PATCH 4/8] mm/damon/core: disallow max < min damos filter range arguments commit

[SJ Park <sj@kernel.org>]

On Sat, 12 Sep 2026 19:29:42 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The newly introduced validation in `damos_commit_filter_arg()` returns `-EINVAL` on failure, but callers `damos_commit_core_filters` and `damos_commit_ops_filters` ignore this return value for new filters, adding uninitialized memory to active DAMON context.
> - [High] `damos_commit_filter()` mutates the target filter's state (type, matching, allow) before validating its arguments, leaving the live filter in a corrupted state if validation subsequently fails.
> --
> 
> commit b7aac655c61845e48301bed4669cfcf294cf84cb
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: disallow max < min damos filter range arguments commit
> 
> This commit adds validation to damos_commit_filter_arg() to reject invalid
> ranges where max is less than min. It returns -EINVAL when an invalid range
> is provided, avoiding duplicated validation across different DAMON API callers.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 3ad1fb2f21f3f..3dec1a8f7ed3a 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1325,15 +1325,21 @@ static int damos_commit_filter_arg(
> >  		dst->memcg_id = src->memcg_id;
> >  		break;
> >  	case DAMOS_FILTER_TYPE_ADDR:
> > +		if (src->addr_range.end < src->addr_range.start)
> > +			return -EINVAL;
> 
> [Severity: High]
> Does this code leave the target filter in a corrupted state on error?
> 
> Looking at damos_commit_filter(), the state is mutated before the newly
> introduced validation has a chance to execute:
> 
> mm/damon/core.c:damos_commit_filter() {
> 	dst->type = src->type;
> 	dst->matching = src->matching;
> 	dst->allow = src->allow;
> 	return damos_commit_filter_arg(dst, src);
> }
> 
> If damos_commit_filter_arg() returns -EINVAL here, dst->type, dst->matching,
> and dst->allow have already been modified. If an existing ops filter receives
> an invalid range update, its type will be updated but its union arguments will
> belong to the previous type. When the filter executes, it might misinterpret
> the old union memory according to the new type, causing unintended matching
> behavior.

Yes, and the caller will destroy it, so no issue.

> 
> 
> [Severity: High]
> Can this validation error cause uninitialized memory to be added to the active
> DAMON context?
> 
> Looking at damos_commit_core_filters() and damos_commit_ops_filters(), the
> return value of damos_commit_filter_arg() is ignored when creating new filters:
> 
> mm/damon/core.c:damos_commit_core_filters() {
> ...
> 		new_filter = damos_new_filter(
> 				src_filter->type, src_filter->matching,
> 				src_filter->allow);
> 		if (!new_filter)
> 			return -ENOMEM;
> 		damos_commit_filter_arg(new_filter, src_filter);
> 		damos_add_filter(dst, new_filter);
> 	}
> 
> Because damos_new_filter() allocates memory without zero-initializing the union
> arguments, returning -EINVAL from damos_commit_filter_arg() means the union
> payload (like addr_range or sz_range) is left uninitialized. The callers ignore
> this error and add the filter to the system anyway, which could result in
> using kmalloc garbage and triggering undefined matching behavior.
> 
> Additionally, damos_commit_ops_filters() ignores the return value of
> damos_commit_filter() for existing ops filters:
> 
> mm/damon/core.c:damos_commit_ops_filters() {
> ...
> 	damos_for_each_ops_filter_safe(dst_filter, next, dst) {
> 		src_filter = damos_nth_ops_filter(i++, src);
> 		if (src_filter)
> 			damos_commit_filter(dst_filter, src_filter);
> ...
> 
> If an existing ops filter receives an invalid update, the validation error
> is swallowed entirely.

Good findings, I will fix this in the next spin.

> 
> >  		dst->addr_range = src->addr_range;
> >  		break;
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260912191807.138502-1-sj@kernel.org?part=4


Thanks,
SJ