Re: [RFC PATCH v2 2/2] mm/damon/lru_sort: error out for >10000 active_mem_bp

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

New issues:
- [High] The newly added validation check for `active_mem_bp` can be bypass=
ed via a Time-Of-Check to Time-Of-Use (TOCTOU) race condition, resulting in=
 division by zero.

Pre-existing issues:
- [High] The function `damos_get_in_active_mem_bp()` triggers a division by=
 zero if the system tracks no active or inactive LRU pages.
- [High] Writing to the `commit_inputs` module parameter while DAMON is ini=
tialized but not running indefinitely suspends the writing process.
--

--- Patch [2]: [RFC PATCH v2 2/2] mm/damon/lru_sort: error out for >10000 a=
ctive_mem_bp ---
commit e683bbcbac1ca0611d9909a3bb95074271d3aa04
Author: SJ Park <[email protected]>

mm/damon/lru_sort: error out for >10000 active_mem_bp

damos_quota_score() can trigger division by zero if the target value is
zero.  DAMON_LRU_SORT lets users set the target value for the hot memory
scheme via active_mem_bp parameter.  It avoids setting it as the target
value if the parameter value is zero.  However, it also sets the cold
memory scheme with a target value that is calculated as '10000 -
active_mem_bp + 2'.  Hence, if a user sets active_mem_bp 10002, the cold
memory scheme's quota goal target value can be zero.  As a result,
division by zero can be triggered.  Fix by returning an error when the
user tries to start DAMON with >10000 active_mem_bp parameter value.

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

> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 7e077084cb03a..3920d5042f109 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -233,6 +233,8 @@ static int damon_lru_sort_add_quota_goals(struct damo=
s *hot_scheme,
> =20
>  	if (!active_mem_bp)
>  		return 0;
> +	if (10000 < active_mem_bp)
> +		return -EINVAL;
>  	goal =3D damos_new_quota_goal(DAMOS_QUOTA_ACTIVE_MEM_BP, active_mem_bp);

[Severity: High]
Since active_mem_bp is a global module parameter, can its value change betw=
een
the bounds check and the call to damos_new_quota_goal()?

If a concurrent write updates active_mem_bp to 10002 immediately after the
check completes, the cold memory scheme's quota goal target value could sti=
ll
end up being zero, resulting in the division by zero this patch aims to
prevent.

Would caching the value locally with READ_ONCE() avoid this race?

>  	if (!goal)
>  		return -ENOMEM;

While reviewing this code, I also noticed a couple of pre-existing issues.

This isn't a bug introduced by this patch, but in mm/damon/core.c:

mm/damon/core.c:damos_get_in_active_mem_bp() {
    ...
	active =3D global_node_page_state(NR_LRU_BASE + LRU_ACTIVE_ANON) +
		global_node_page_state(NR_LRU_BASE + LRU_ACTIVE_FILE);
	inactive =3D global_node_page_state(NR_LRU_BASE + LRU_INACTIVE_ANON) +
		global_node_page_state(NR_LRU_BASE + LRU_INACTIVE_FILE);
	total =3D active + inactive;
	if (active_ratio)
		return mult_frac(active, 10000, total);
	return mult_frac(inactive, 10000, total);
}

[Severity: High]
Can mult_frac() trigger a division by zero if total is 0?

If the system tracks no active or inactive LRU pages, or if per-CPU counter
drifts clamp the global state to 0, total will evaluate to 0 and be used as
the denominator.


This is also a pre-existing issue, but in mm/damon/lru_sort.c:

mm/damon/lru_sort.c:damon_lru_sort_commit_inputs_store() {
    ...
	/*
	 * Skip damon_call() if ctx is not initialized to avoid
	 * NULL pointer dereference.
	 */
	if (!ctx)
		return -EINVAL;

	err =3D damon_call(ctx, &control);
    ...
}

[Severity: High]
Does damon_call() require the kdamond thread to be actively running?

If the module is initialized but DAMON has not been enabled, it appears
damon_call() will queue the request and wait indefinitely on a completion
that will never be signaled by the stopped thread, causing the writing
process to hang in an uninterruptible sleep state.

Could we check damon_is_running(ctx) before calling damon_call()?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260802162050.8947=
[email protected]?part=3D2