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

[SJ Park <sj@kernel.org>]

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

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

Yes, and the caller will destroy the filter, so no problem.

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

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

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

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

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


Thanks,
SJ