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

[SeongJae Park]

On Sun, 29 Mar 2026 08:34:06 -0700 SeongJae Park <sj@xxxxxxxxxx> wrote:

> Forwarding Sashiko review for doing discussions via mails.
> 
> # review url: https://sashiko.dev/#/patchset/20260329043902.46163-2-sj@xxxxxxxxxx
> # start of sashiko.dev inline review
> commit 8c40c45c57722526c25cf1114b386d19a693fc2d
> 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 node ID in damos_quota_goal to prevent
> out of bounds memory access when calling si_meminfo_node(). If an invalid
> node ID is given, it defaults to returning 0% for used memory and 100% for
> free memory.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> >  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);
> 
> Does this code still risk a divide-by-zero kernel panic if a memory node
> has zero managed pages?
> 
> Even with the new invalid_mem_node() check ensuring node_state(nid, N_MEMORY)
> is true, a memory node can have present physical pages but zero managed pages
> if all its memory is reserved early in boot (for example, via memblock_reserve
> for crash kernels or firmware).
> 
> In this scenario, si_meminfo_node() sets i.totalram to 0. Since the
> mult_frac() macro directly evaluates division and modulo, passing
> i.totalram as 0 will cause a hardware divide-by-zero exception.
> 
> Should there be an explicit check for i.totalram == 0 before calling
> mult_frac() here and in other similar quota functions like
> damos_get_node_memcg_used_bp()?

As I mentioned to same previous comment, that's another bug that require
another patch.  I will work on it with a separate patch.


Thanks,
SJ

[...]