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

[SeongJae Park]

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


# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/20260329043902.46163-2-sj@xxxxxxxxxx
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260329043902.46163-2-sj@xxxxxxxxxx
#
# [1] https://github.com/sjp38/hackermail

Sent using hkml (https://github.com/sjp38/hackermail)