mm/damon: avoid division by zero from damos_quota_score()

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

New issues:
- [Low] The newly added comment incorrectly refers to damos_quota_store() instead of damos_quota_score().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the division-by-zero vulnerability is only patched at the module boundary in mtier.c. The unvalidated core API (damos_new_quota_goal and damos_quota_score) remains vulnerable and is still exploitable via other DAMON components. For instance, in mm/damon/lru_sort.c, an unchecked active_mem_bp module parameter can be set via sysfs such that it evaluates to 0, causing a kernel panic. --

--- Patch [1]: [RFC PATCH v2 1/2] samples/damon/mtier: error out for zero quota goal target values --- commit a29006e1d33ea981b557928c622fb8487e53dbc8
Author: SJ Park <[email protected]>
samples/damon/mtier: error out for zero quota goal target values

This commit prevents a division-by-zero exception in damos_quota_score() by returning an error when users try to start DAMON with zero values for the node0_mem_used_bp or node0_mem_free_bp parameters.

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

samples/damon/mtier.c static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote) · line 159

Quoted 9 lines

samples/damon/mtier.c +1 -0

index ac9c24b92ead8..d1123ebbfab90 100644
@@ -156,6 +156,9 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
 	if (!scheme)
 		goto free_out;
 	damon_set_schemes(ctx, &scheme, 1);
+	/* zero target value causes division by zero in damos_quota_store() */

[Severity: Low]
This isn't a bug, but should this refer to damos_quota_score() instead of damos_quota_store()?

samples/damon/mtier.c static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote) · line 162

Quoted 4 lines

samples/damon/mtier.c +2 -0

+	if (!node0_mem_used_bp || !node0_mem_free_bp)
+		goto free_out;
 	quota_goal = damos_new_quota_goal(

[Severity: High]
This isn't a bug introduced by this patch, but while this change handles the zero target value at the module boundary in mtier.c, does the unvalidated core API leave other DAMON components vulnerable?

For example, looking at damon_lru_sort_add_quota_goals() in mm/damon/lru_sort.c:

goal = damos_new_quota_goal(DAMOS_QUOTA_INACTIVE_MEM_BP,
10000 - active_mem_bp + 2);

If active_mem_bp is set to 10002 via sysfs, this evaluates to 0.

Because damos_new_quota_goal() in mm/damon/core.c accepts the target_value without validation, this zero value propagates to damos_quota_score() where mult_frac() performs a division by goal->target_value.

Could this cause a division by zero exception leading to a kernel panic? Would it be safer to add validation directly inside damos_new_quota_goal() or damos_quota_score() to protect all consumers?

Quoted 4 lines

samples/damon/mtier.c +0 -0

 			promote ? DAMOS_QUOTA_NODE_MEM_USED_BP :
 			DAMOS_QUOTA_NODE_MEM_FREE_BP,

promote ? node0_mem_used_bp : node0_mem_free_bp);

Signature · 1 line

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

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 bypassed 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 initialized but not running indefinitely suspends the writing process. --

--- Patch [2]: [RFC PATCH v2 2/2] mm/damon/lru_sort: error out for >10000 active_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] open here

mm/damon/lru_sort.c static int damon_lru_sort_add_quota_goals(struct damos *hot_scheme, · line 238

Quoted 11 lines

mm/damon/lru_sort.c +2 -0

index 7e077084cb03a..3920d5042f109 100644
@@ -233,6 +233,8 @@ static int damon_lru_sort_add_quota_goals(struct damos *hot_scheme,
 
 	if (!active_mem_bp)
 		return 0;
+	if (10000 < active_mem_bp)
+		return -EINVAL;
 	goal = 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 between 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 still 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?

mm/damon/lru_sort.c static int damon_lru_sort_add_quota_goals(struct damos *hot_scheme, · line 240

Quoted 3 lines

mm/damon/lru_sort.c +0 -0

 	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 = global_node_page_state(NR_LRU_BASE + LRU_ACTIVE_ANON) +
		global_node_page_state(NR_LRU_BASE + LRU_ACTIVE_FILE);
	inactive = global_node_page_state(NR_LRU_BASE + LRU_INACTIVE_ANON) +
		global_node_page_state(NR_LRU_BASE + LRU_INACTIVE_FILE);
	total = 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 = 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()?

Signature · 2 lines

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