Re: [PATCH v9.1] mm/damon: add node_eligible_mem_bp goal metric

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Data truncation and arithmetic overflow in metric calculation on 32-bit systems.
- [High] Missing `cond_resched()` inside the inner PFN-walking loop causes CPU soft lockups.
- [High] Address advancement logic is vulnerable to wrap-around infinite loops.
--

commit 10100659661304515a414f3a15324e8278667f42
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>

mm/damon: add node_eligible_mem_bp goal metric

This commit introduces the DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP goal metric to
track the ratio of scheme-eligible memory on a specific NUMA node, allowing
the quota autotuner to adjust based on spatial memory distribution.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 3dbbbfdeff71..22d6eb3a974d 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> +static phys_addr_t damos_calc_eligible_bytes(struct damon_ctx *c,
> +		struct damos *s, int nid, phys_addr_t *total)
> +{
[ ... ]
> +			/* Convert from core address units to physical bytes */
> +			addr = (phys_addr_t)r->ar.start * c->addr_unit;
> +			end_addr = (phys_addr_t)r->ar.end * c->addr_unit;
> +			while (addr < end_addr) {
> +				struct folio *folio;
> +				phys_addr_t folio_start, folio_end;
> +				phys_addr_t overlap_start, overlap_end;
> +				phys_addr_t counted;
> +
> +				folio = damon_get_folio(PHYS_PFN(addr));
> +				if (!folio) {
> +					addr = PAGE_ALIGN_DOWN(addr + PAGE_SIZE);

If a DAMON region extends to the upper boundary of the physical address space,
could addr + PAGE_SIZE overflow and wrap around to 0?

If addr wraps to 0, addr < end_addr would remain true, which might cause an
unrecoverable infinite loop.

> +					continue;
> +				}
[ ... ]
> +				/* Advance past the entire folio */
> +				addr = folio_end;
> +				folio_put(folio);
> +			}

Because DAMON regions can span tens of gigabytes, could this inner loop
execute millions of times sequentially without yielding?

The cond_resched() is placed outside this loop, which might trigger CPU soft
lockups if a very large region is processed. Would it be better to move
cond_resched() inside the while loop?

> +			cond_resched();
> +		}
> +	}
> +
> +	*total = total_eligible;
> +	return node_eligible;
> +}
> +
> +static unsigned long damos_get_node_eligible_mem_bp(struct damon_ctx *c,
> +		struct damos *s, int nid)
> +{
> +	phys_addr_t total_eligible = 0;
> +	phys_addr_t node_eligible;
> +
> +	if (c->ops.id != DAMON_OPS_PADDR)
> +		return 0;
> +
> +	if (nid < 0 || nid >= MAX_NUMNODES || !node_online(nid))
> +		return 0;
> +
> +	node_eligible = damos_calc_eligible_bytes(c, s, nid, &total_eligible);
> +
> +	if (!(unsigned long)total_eligible)
> +		return 0;
> +
> +	return mult_frac((unsigned long)node_eligible, 10000,
> +			(unsigned long)total_eligible);
> +}

On 32-bit PAE systems, phys_addr_t is 64-bit while unsigned long is 32-bit.
Does casting total_eligible and node_eligible to unsigned long discard the
upper 32 bits?

If total_eligible happens to be exactly a multiple of 4 GB, the truncation
could result in 0, causing the function to unexpectedly bypass the check.

Furthermore, mult_frac() performs intermediate multiplication. Even if the
sizes fit in 32 bits, could the intermediate calculation overflow the 32-bit
maximum when node_eligible exceeds ~429 KB?

Would it be safer to evaluate the byte counts using 64-bit arithmetic, such
as div64_u64(), to prevent truncation and overflow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260427010209.843-1-ravis.opensrc@gmail.com?part=1