Re: (sashiko review) [RFC PATCH 1/2] mm/damon/core: validate damos_quota_goal->nid for node_mem_{used,free}_bp

[SeongJae Park]

TL;DR: good catch but orthogonal to this patch.

On Fri, 27 Mar 2026 19:29:55 -0700 SeongJae Park <sj@xxxxxxxxxx> wrote:

> Forwarding Sashiko review for doing discussions via mails.
> 
> # review url: https://sashiko.dev/#/patchset/20260328005412.7606-2-sj@xxxxxxxxxx
> # start of sashiko.dev inline review
> commit add63112781ae04626ae6cb9bce55a85234f6e49
> Author: SeongJae Park <sj@xxxxxxxxxx>
> 
> mm/damon/core: validate damos_quota_goal->nid for node_mem_{used,free}_bp
> 
> This commit adds validation for the given memory node id to prevent out of
> bounds access when calculating node memory metrics. If an invalid node id
> is provided, it defaults to returning 0% used or 100% free memory.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> > @@ -2239,12 +2239,24 @@ static inline u64 damos_get_some_mem_psi_total(void)
> >  static __kernel_ulong_t damos_get_node_mem_bp(
> >  		struct damos_quota_goal *goal)
> >  {
> >  	struct sysinfo i;
> >  	__kernel_ulong_t numerator;
> >  
> > +	if (invalid_mem_node(goal->nid)) {
> > +		if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
> > +			return 0;
> > +		else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
> > +			return 10000;
> > +	}
> > +
> >  	si_meminfo_node(&i, goal->nid);
> >  	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
> >  		numerator = i.totalram - i.freeram;
> >  	else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
> >  		numerator = i.freeram;
> >  	return mult_frac(numerator, 10000, i.totalram);
> 
> Could this result in a division by zero panic?
> 
> While invalid_mem_node() checks node_state(nid, N_MEMORY) to ensure the
> node has present memory pages, si_meminfo_node() sets i.totalram based
> solely on managed pages. If all memory on the node is reserved, i.totalram
> could be 0.
> 
> Additionally, since the node_state() check is lockless, memory could be
> concurrently offlined after invalid_mem_node() but before si_meminfo_node(),
> also resulting in i.totalram being 0.
> 
> Would it be safer to explicitly verify i.totalram is greater than 0 before
> calling mult_frac()?

I'm not very sure if totalram becoming zero is available, but seems making
sense.  But orthogonal to this patch.  I will work on this with another patch.


Thanks,
SJ

[...]