Re: [RFC PATCH 3/8] mm/damon/core: return an error from damos_commit_filter_arg()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unhandled error returns in DAMOS filter commit functions lead to uninitialized memory use and type confusion.
--

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

mm/damon/core: return an error from damos_commit_filter_arg()

damos_commit_filter_arg() is supposed to always succeed.  It may not in
future, for example, if the given filter is invalid.  Prepare the case
by modifying its signature to return an error when it failed.  Also pipe
the return value to its callers and let them handle the error.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 0c98f7e267d07..3ad1fb2f21f3f 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> -static void damos_commit_filter(
> +static int damos_commit_filter(
>  		struct damos_filter *dst, struct damos_filter *src)
>  {
>  	dst->type = src->type;
>  	dst->matching = src->matching;
>  	dst->allow = src->allow;
> -	damos_commit_filter_arg(dst, src);
> +	return damos_commit_filter_arg(dst, src);
>  }

[Severity: High]
Does this code leave the target filter in a type-confused state if
damos_commit_filter_arg() fails?

Since dst->type is modified before the arguments are committed, if an error
is returned and subsequently ignored by the caller, the filter could have a
new type but mismatched union arguments from the old type.

>  static int damos_commit_core_filters(struct damos *dst, struct damos *src)
>  {
>  	struct damos_filter *dst_filter, *next, *src_filter, *new_filter;
> -	int i = 0, j = 0;
> +	int i = 0, j = 0, err;
>  
>  	damos_for_each_core_filter_safe(dst_filter, next, dst) {
>  		src_filter = damos_nth_core_filter(i++, src);
> -		if (src_filter)
> -			damos_commit_filter(dst_filter, src_filter);
> -		else
> +		if (src_filter) {
> +			err = damos_commit_filter(dst_filter, src_filter);
> +			if (err)
> +				return err;
> +		} else {
>  			damos_destroy_filter(dst_filter);
> +		}
>  	}
>  
>  	damos_for_each_core_filter_safe(src_filter, next, src) {

[Severity: High]
Does this code ignore the return value of damos_commit_filter_arg() when
creating new core filters?

Further down in this loop:

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);
	...
}

If damos_commit_filter_arg() fails, does this silently add an uninitialized
filter with garbage union arguments to the destination scheme?

[Severity: High]
Are there also missing error checks in damos_commit_ops_filters()?

It appears damos_commit_ops_filters() wasn't updated in this patch to
check the new integer return values from damos_commit_filter() or
damos_commit_filter_arg():

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);
	...
	damos_for_each_ops_filter_safe(src_filter, next, src) {
		...
		damos_commit_filter_arg(new_filter, src_filter);
		damos_add_filter(dst, new_filter);
	...
}

Can ignoring these errors leave the ops filter in a type-confused state or
silently add uninitialized filters to the destination scheme?

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