Re: [PATCH 3/4] mm/damon/core: copy nid for eligible_mem_bp damos quota goal commit

[SJ Park <sj@kernel.org>]

On Wed, 02 Sep 2026 00:39:50 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Inconsistent state handling in damon_commit_ctx() on memory allocation failure.
> --
> 
> commit c0fdc35d2fb6726e726ba0e857811cbaefc34918
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: copy nid for eligible_mem_bp damos quota goal commit
> 
> damos_commit_quota_goal_union() is not updating the ->nid union field
> when the goal metric is DAMOS_QUITA_NODE_ELIGIBLE_MEM_BP.  Hence, if a
> DAMOS quota goal of the type is online committed in a way that it will
> reuse other quota goal's memory space, the new goal will work with a
> garbage nid value.  As a result, the DAMOS scheme can show unexpected
> aggressiveness.
> 
> > damos_commit_quota_goal_union() is not updating the ->nid union field
> > when the goal metric is DAMOS_QUITA_NODE_ELIGIBLE_MEM_BP.
> 
> This isn't a bug, but there's a typo (DAMOS_QUITA_NODE_ELIGIBLE_MEM_BP) in
> the commit message.

Good catch.  I will ask Andrew if he could fix this while picking this.

> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 647ac77f573bb..a499d7866c507 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1143,6 +1143,9 @@ static void damos_commit_quota_goal_union(
> >  		dst->nid = src->nid;
> >  		dst->memcg_id = src->memcg_id;
> >  		break;
> > +	case DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP:
> > +		dst->nid = src->nid;
> > +		break;
> >  	default:
> >  		break;
> >  	}
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but is a pre-existing logic
> flaw in the core DAMON commit mechanism on memory allocation failure.
> 
> In mm/damon/core.c:damon_commit_ctx():
> 
> 	err = __damon_commit_ctx(test_ctx, src);
> 	if (err)
> 		goto out;
> 	err = __damon_commit_ctx(dst, src);
> 
> If the second call to __damon_commit_ctx() fails with -ENOMEM, won't it
> leave the active dst context corrupted?
> 
> For example, an OOM condition can occur here in
> mm/damon/core.c:damon_commit_schemes():
> 
> 		new_scheme = damon_new_scheme(&src_scheme->pattern,
> 				src_scheme->action,
> 				src_scheme->apply_interval_us,
> 				&src_scheme->quota, &src_scheme->wmarks,
> 				NUMA_NO_NODE);
> 		if (!new_scheme)
> 			return -ENOMEM;
> 
> A failure here exits the commit process without rolling back prior scheme
> updates, leaving the active dst context partially updated.
> 
> The kernel handles this by setting dst->maybe_corrupted = true, which
> forces the kdamond monitoring thread to terminate gracefully but permanently.
> 
> Could this be triggered by a user-initiated online parameter update
> encountering an OOM condition during the second __damon_commit_ctx()
> invocation?
> 
> Does this violate the atomic nature of damon_commit_ctx() (which uses
> test_ctx precisely to avoid partial updates on the active context) and
> cause an unintended loss of the running context?

Such user would need to have sysfs write permission first.  With the
permission, the user could do whatever, even worse than stopping DAMON.

So no issue in my opinion.

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


Thanks,
SJ