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

[sashiko-bot@kernel.org]

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.


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

[ ... ]

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