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

[sashiko-bot@kernel.org]

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.

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

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