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

SeongJae Park
2026-03-28T13:32:17Z

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

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

> Forwarding Sashiko review for doing discussions via mails.
>
> # review url: https://sashiko.dev/#/patchset/20260328005412.7606-2-sj@kernel.org
> # start of sashiko.dev inline review
> commit add63112781ae04626ae6cb9bce55a85234f6e49
> Author: SeongJae Park <sj@kernel.org>
>
> 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 …