Re: [RFC PATCH 8/8] mm/damon/core-kunit: test invalid damos filter commits

[SJ Park <sj@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Missing error handling for damos_commit_filter_arg() and damos_commit_filter() leads to uninitialized memory use for DAMON filters.
> 
> Pre-existing issues:
> - [Critical] Memory allocation failure during damos_commit_dests() leaves the active scheme object partially torn down, leading to a deterministic NULL pointer dereference.
> --
> 
> commit 0ffd278b1f3947b2729c57dd026fc9283304d3af
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core-kunit: test invalid damos filter commits
>     
> Add test cases for testing the validation of damos filter arguments in
> commit time.
> 
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index 2b0931cf6fb32..527abc2570616 100644
> > --- a/mm/damon/tests/core-kunit.h
> > +++ b/mm/damon/tests/core-kunit.h
> > @@ -1184,6 +1184,13 @@ static void damos_test_commit_filter(struct kunit *test)
> >  			.allow = false,
> >  			.sz_range = {.min = 234, .max = 345},
> >  			}, false);
> > +	damos_test_commit_filter_for(test, &dst,
> > +			&(struct damos_filter){
> > +			.type = DAMOS_FILTER_TYPE_HUGEPAGE_SIZE,
> > +			.matching = false,
> > +			.allow = false,
> > +			.sz_range = {.min = 456, .max = 123},
> > +			}, true);
> 
> [Severity: High]
> Does this patch series leave uninitialized memory in the payload of DAMON
> filters if an invalid range is provided?
> 
> While this patch adds tests for invalid filter ranges, it appears the error
> handling in earlier patches of the series might be incomplete. If a user
> provides an invalid range (e.g., max < min) via DAMON sysfs, the core
> validation returns -EINVAL, but this error is ignored when allocating a new
> core filter:
> 
> mm/damon/core.c:damos_commit_core_filters() {
>     ...
> 	damos_for_each_core_filter_safe(src_filter, next, src) {
> 		if (j++ < i)
> 			continue;
> 
> 		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);
> 	}
>     ...
> }
> 
> Similarly, the return values of both damos_commit_filter() and
> damos_commit_filter_arg() are ignored when updating or allocating an ops
> filter:
> 
> 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);
> 		else
> 			damos_destroy_filter(dst_filter);
> 	}
> 
> 	damos_for_each_ops_filter_safe(src_filter, next, src) {
> 		if (j++ < i)
> 			continue;
> 
> 		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);
> 	}
>     ...
> }
> 
> Since damos_new_filter() allocates with kmalloc_obj() (which does not zero
> memory), ignoring the return value of damos_commit_filter_arg() could lead to
> the uninitialized heap data remaining in the filter's payload
> (e.g. sz_range or addr_range) when it is added to the active DAMON context.

Yes, I will fix in the next spin.

> 
> 
> [Severity: Critical]
> This is a pre-existing issue, but can memory allocation failure during
> damos_commit_dests() leave the active scheme partially torn down and lead to a
> NULL pointer dereference?
> 
> If memory allocation fails for dst->weight_arr, damos_commit_dests() leaves
> dst->nr_dests with its old non-zero value, but assigns NULL to the array:
> 
> mm/damon/core.c:damos_commit_dests() {
>     ...
> 		dst->weight_arr = kmalloc_array(src->nr_dests,
> 			sizeof(*dst->weight_arr), GFP_KERNEL);
> 		if (!dst->weight_arr) {
> 			/* ->node_id_arr will be freed by scheme destruction */
> 			return -ENOMEM;
> 		}
>     ...
> }
> 
> Because damon_commit_schemes() breaks its loop on -ENOMEM and returns, the
> partially modified scheme is left physically linked in the live context's
> scheme list:
> 
> mm/damon/core.c:damon_commit_schemes() {
>     ...
> 		src_scheme = damon_nth_scheme(i++, src);
> 		if (src_scheme) {
> 			err = damos_commit(dst_scheme, src_scheme);
> 			if (err)
> 				return err;
> 		}
>     ...
> }
> 
> When the kdamond worker thread later executes damos_va_migrate_dests_add() on
> this corrupted scheme, it appears it will iterate up to dests->nr_dests and
> dereference the NULL weight_arr pointer:
> 
> mm/damon/vaddr.c:damos_va_migrate_dests_add() {
>     ...
> 	for (i = 0; i < dests->nr_dests; i++)
> 		weight_total += dests->weight_arr[i];
>     ...
> }
> 
> Is there a mechanism preventing this kernel panic if memory allocation
> fails on the live destination context?

Yes, we have damon_ctx->maybe_corrupted to prevent this.  So, no issue.

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


Thanks,
SJ