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

On Sun, 02 Aug 2026 16:36:51 +0000 [email protected] wrote:

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

Such race cannot happen to my understanding, thanks to the param_lock.

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

Known issue.  I'm working on it.

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

Nice finding.  I will separately work on this.

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


Thanks,
SJ