Re: [PATCH v3 1/3] mm/damon: Introduce DAMOS_QUOTA_HUGEPAGE auto tuning

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Unsigned underflow in `total` bypasses the division-by-zero guard, causing `damos_hugepage_mem_bp` to return 0 instead of the intended safeguard `INT_MAX`.
- [Medium] The new `DAMOS_QUOTA_HUGEPAGE_MEM_BP` metric is missing from the sysfs parser mapping array, rendering the feature completely inaccessible to userspace.
- [Medium] 32-bit integer overflow in `mult_frac()` yields drastically incorrect ratios when the number of huge pages exceeds ~1.7GB.

Pre-existing issues:
- [High] Preexisting division by zero in `damos_get_in_active_mem_bp` when LRU lists are empty.
--

--- Patch [1]: [PATCH v3 1/3] mm/damon: Introduce DAMOS_QUOTA_HUGEPAGE auto tuning ---
commit 8d24f093924a5075fd67c8f7808eada2166af60d
Author: Asier Gutierrez <[email protected]>

mm/damon: Introduce DAMOS_QUOTA_HUGEPAGE auto tuning

Introduce DAMOS_QUOTA_HUGEPAGE_MEM_BP auto tuning and add a new DAMOS
quota goal metric to measure huge page consumption. Add a guard to avoid
division by 0 when vmstat lags.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/include/linux/damon.h b/include/linux/damon.h
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
[ ... ]
> @@ -167,6 +168,7 @@ enum damos_quota_goal_metric {
>  	DAMOS_QUOTA_ACTIVE_MEM_BP,
>  	DAMOS_QUOTA_INACTIVE_MEM_BP,
>  	DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP,
> +	DAMOS_QUOTA_HUGEPAGE_MEM_BP,

[Severity: Medium]
Is this feature accessible from userspace?

If a user writes hugepage_mem_bp to the target_metric sysfs file,
target_metric_store() iterates over the damos_sysfs_qgoal_metric_names
array to find a match.

Because DAMOS_QUOTA_HUGEPAGE_MEM_BP is missing from that array in
mm/damon/sysfs-schemes.c, will the kernel always reject the user's input
with -EINVAL?

>  	NR_DAMOS_QUOTA_GOAL_METRICS,
>  };
>  
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3007,6 +3007,24 @@ static unsigned int damos_get_in_active_mem_bp(bool active_ratio)
>  	return mult_frac(inactive, 10000, total);

[Severity: High]
This is a pre-existing issue, but can this divide by zero?

In damos_get_in_active_mem_bp(), total is used as the divisor without a zero
check. If all memory is forced onto the unevictable list (e.g. via
mlockall()), or if the system is completely idle with only kernel allocations,
active + inactive can be zero.

If DAMOS_QUOTA_ACTIVE_MEM_BP is used in such a state, would mult_frac()
trigger a hardware divide-by-zero exception and panic the system?

>  }
>  
> +static unsigned int damos_hugepage_mem_bp(void)
> +{
> +	unsigned long thp, total;
> +
> +	thp = global_node_page_state(NR_ANON_THPS) +
> +				global_node_page_state(NR_SHMEM_THPS) +
> +				global_node_page_state(NR_FILE_THPS);
> +	total = totalram_pages() - global_zone_page_state(NR_FREE_PAGES);
> +
> +	/*
> +	 * vmstat may lag in idle systems and lead to free pages equaling to
> +	 * the total RAM in the system.
> +	 */
> +	if (!total)
> +		return INT_MAX;

[Severity: High]
Can total underflow here?

Because global_zone_page_state(NR_FREE_PAGES) relies on vmstat, which uses
per-CPU caching, the global NR_FREE_PAGES counter can temporarily exceed
totalram_pages() if some CPUs have flushed frees while others have pending
allocations.

If this happens, total (which is unsigned long) underflows to a value near
ULONG_MAX. Could this bypass the !total guard and pass a massive integer
to mult_frac(), which then incorrectly returns 0?

> +	return mult_frac(thp, 10000, total);

[Severity: Medium]
Will this overflow on 32-bit architectures?

mult_frac() evaluates the intermediate multiplication using the types of its
arguments. Since thp is an unsigned long, evaluating thp * 10000 uses 32-bit
arithmetic on 32-bit systems like ARM LPAE.

If the system has more than 429,496 base pages of THP (approximately 1.7GB),
will this silently wrap around and yield drastically incorrect quota goal
ratios?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1