Skip to contentSkip to conversation

mm/damon/core: validate goal nid before accessing node data

PATCH Superseded Josh Law 2 participants 5 messages Mar 25

Subsystems (2)
Memory management DAMON

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Josh Law

author
#0 lore raw

PATCH 1/1 mm/damon/core: validate goal nid before accessing node data

damos_get_node_mem_bp() and damos_get_node_memcg_used_bp() pass goal->nid directly to si_meminfo_node() and NODE_DATA() without checking that it refers to a valid, online NUMA node. Since goal->nid is set from userspace via sysfs with no validation, a negative or out-of-range value causes an out-of-bounds access in NODE_DATA(), and a valid but offline node gives undefined results.

Add bounds and node_online() checks before using the nid.

Fixes 0e1c773b501f ("mm/damon/core: introduce damos quota goal metrics for memory node utilization")
Signed-off-by Josh Law <[email protected]>
---
 mm/damon/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/damon/core.c +8 -0
index 59b709f04975..6ee421141996 100644
@@ -2227,6 +2227,10 @@ static __kernel_ulong_t damos_get_node_mem_bp(
 	struct sysinfo i;
 	__kernel_ulong_t numerator;
 
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))
+		return 0;
+
 	si_meminfo_node(&i, goal->nid);
 	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
 		numerator = i.totalram - i.freeram;

@@ -2243,6 +2247,10 @@ static unsigned long damos_get_node_memcg_used_bp(
 	unsigned long used_pages, numerator;
 	struct sysinfo i;
 
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))
+		return 0;
+
 	memcg = mem_cgroup_get_from_id(goal->memcg_id);
 	if (!memcg) {
 		if (goal->metric == DAMOS_QUOTA_NODE_MEMCG_USED_BP)
Signature · 1 line
2.34.1
maintainer
#1 lore raw
Quoted 6 lines · On Wed, 25 Mar 2026 07:30:34 +0000 Josh Law <[email protected]> wrote:

damos_get_node_mem_bp() and damos_get_node_memcg_used_bp() pass goal->nid directly to si_meminfo_node() and NODE_DATA() without checking that it refers to a valid, online NUMA node. Since goal->nid is set from userspace via sysfs with no validation, a negative or out-of-range value causes an out-of-bounds access in NODE_DATA(), and a valid but offline node gives undefined results.

Nice catch!

Quoted 4 lines

Add bounds and node_online() checks before using the nid.

Fixes 0e1c773b501f ("mm/damon/core: introduce damos quota goal metrics for memory node utilization")

Let's add Cc: stable.

Quoted 10 lines
Signed-off-by Josh Law <[email protected]>
---
 mm/damon/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/damon/core.c +0 -0
index 59b709f04975..6ee421141996 100644

mm/damon/core.c static __kernel_ulong_t damos_get_node_mem_bp( · line 2231

Quoted 5 lines
mm/damon/core.c +2 -0
@@ -2227,6 +2227,10 @@ static __kernel_ulong_t damos_get_node_mem_bp(
 	struct sysinfo i;
 	__kernel_ulong_t numerator;
 
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))

Like damon_migrate_pages(), how about using node_state(goal->nid, N_MEMORY) insted of node_online()?

mm/damon/core.c static unsigned long damos_get_node_memcg_used_bp( · line 2251

Quoted 11 lines
mm/damon/core.c +4 -0
+		return 0;
+
 	si_meminfo_node(&i, goal->nid);
 	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
 		numerator = i.totalram - i.freeram;

@@ -2243,6 +2247,10 @@ static unsigned long damos_get_node_memcg_used_bp(
 	unsigned long used_pages, numerator;
 	struct sysinfo i;
 
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))

Ditto.

Quoted 7 lines
mm/damon/core.c +2 -0
+		return 0;
+
 	memcg = mem_cgroup_get_from_id(goal->memcg_id);
 	if (!memcg) {
 		if (goal->metric == DAMOS_QUOTA_NODE_MEMCG_USED_BP)
Signature · 1 line
2.34.1

Thanks,
SJ

maintainer
#2 lore raw

Re: (sashiko review) [PATCH] mm/damon/core: validate goal nid before accessing node data

In reply to Josh Law

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit 7e17cbe1dc95a0cd1a1410be38ab6964f78fb15c
Author: Josh Law <[email protected]>

mm/damon/core: validate goal nid before accessing node data

This commit adds bounds checking and node_online() validation for the goal->nid in the DAMON quota metrics. It prevents out-of-bounds accesses to NODE_DATA() when an invalid or offline NUMA node is provided by userspace.

Quoted 15 lines
mm/damon/core.c +4 -0
index db6c67e52d2b..81ce27baa1a1 100644
@@ -2245,6 +2245,10 @@ static __kernel_ulong_t damos_get_node_mem_bp(
 	struct sysinfo i;
 	__kernel_ulong_t numerator;
 
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))
+		return 0;
+
 	si_meminfo_node(&i, goal->nid);
 	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
 		numerator = i.totalram - i.freeram;

mm/damon/core.c

Quoted 3 lines
mm/damon/core.c +0 -0
 	else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
 		numerator = i.freeram;
 	return mult_frac(numerator, 10000, i.totalram);

Could this cause a division by zero if the node is online but memoryless?

For CPU-only compute nodes or nodes where all memory has been hot-removed, node_online(goal->nid) will evaluate to true. When si_meminfo_node() is called for these memoryless nodes, it populates the sysinfo struct with i.totalram = 0.

The mult_frac() macro unconditionally performs integer division and modulo by the third argument, which could trigger a kernel panic when dividing by zero.

Would it be safer to check if i.totalram is zero before returning, or use node_state(goal->nid, N_MEMORY) instead of node_online()?

[ ... ]

mm/damon/core.c static unsigned long damos_get_node_memcg_used_bp( · line 2272

Quoted 9 lines
mm/damon/core.c +4 -0
@@ -2261,6 +2265,10 @@ static unsigned long damos_get_node_memcg_used_bp(
 	unsigned long used_pages, numerator;
 	struct sysinfo i;
 
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))
+		return 0;
+
 	memcg = mem_cgroup_get_from_id(goal->memcg_id);

The same division by zero risk exists here when mult_frac() is called at the end of this function.

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

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

maintainer
#3 lore raw

Re: (sashiko review) [PATCH] mm/damon/core: validate goal nid before accessing node data

Quoted 52 lines · On Wed, 25 Mar 2026 07:37:39 -0700 SeongJae Park <[email protected]> wrote:

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit 7e17cbe1dc95a0cd1a1410be38ab6964f78fb15c
Author: Josh Law <[email protected]>

mm/damon/core: validate goal nid before accessing node data

This commit adds bounds checking and node_online() validation for the goal->nid in the DAMON quota metrics. It prevents out-of-bounds accesses to NODE_DATA() when an invalid or offline NUMA node is provided by userspace.

Quoted 15 lines
mm/damon/core.c +4 -0
index db6c67e52d2b..81ce27baa1a1 100644
@@ -2245,6 +2245,10 @@ static __kernel_ulong_t damos_get_node_mem_bp(
 	struct sysinfo i;
 	__kernel_ulong_t numerator;
 
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))
+		return 0;
+
 	si_meminfo_node(&i, goal->nid);
 	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
 		numerator = i.totalram - i.freeram;

mm/damon/core.c

Quoted 3 lines
mm/damon/core.c +0 -0
 	else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
 		numerator = i.freeram;
 	return mult_frac(numerator, 10000, i.totalram);

Could this cause a division by zero if the node is online but memoryless?

For CPU-only compute nodes or nodes where all memory has been hot-removed, node_online(goal->nid) will evaluate to true. When si_meminfo_node() is called for these memoryless nodes, it populates the sysinfo struct with i.totalram = 0.

The mult_frac() macro unconditionally performs integer division and modulo by the third argument, which could trigger a kernel panic when dividing by zero.

Would it be safer to check if i.totalram is zero before returning, or use node_state(goal->nid, N_MEMORY) instead of node_online()?

[ ... ]

Quoted 4 lines
mm/damon/core.c +0 -0
@@ -2261,6 +2265,10 @@ static unsigned long damos_get_node_memcg_used_bp(
 	unsigned long used_pages, numerator;
 	struct sysinfo i;
Quoted 8 lines

mm/damon/core.c static unsigned long damos_get_node_memcg_used_bp( · line 2254

Quoted 5 lines
mm/damon/core.c +4 -0
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))
+		return 0;
+
 	memcg = mem_cgroup_get_from_id(goal->memcg_id);

The same division by zero risk exists here when mult_frac() is called at the end of this function.

I think sashiko's comments make sense. I also suggested using node_state(N_MEMORY). What do you think, Josh?

Thanks,
SJ

[...]

author
#4 lore raw

Re: (sashiko review) [PATCH] mm/damon/core: validate goal nid before accessing node data

Quoted 63 lines · SeongJae Park
Quoted 52 lines · On Wed, 25 Mar 2026 07:37:39 -0700 SeongJae Park <[email protected]> wrote:

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit 7e17cbe1dc95a0cd1a1410be38ab6964f78fb15c
Author: Josh Law <[email protected]>

mm/damon/core: validate goal nid before accessing node data

This commit adds bounds checking and node_online() validation for the goal->nid in the DAMON quota metrics. It prevents out-of-bounds accesses to NODE_DATA() when an invalid or offline NUMA node is provided by userspace.

Quoted 15 lines
mm/damon/core.c +4 -0
index db6c67e52d2b..81ce27baa1a1 100644
@@ -2245,6 +2245,10 @@ static __kernel_ulong_t damos_get_node_mem_bp(
 	struct sysinfo i;
 	__kernel_ulong_t numerator;
 
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))
+		return 0;
+
 	si_meminfo_node(&i, goal->nid);
 	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
 		numerator = i.totalram - i.freeram;

mm/damon/core.c

Quoted 3 lines
mm/damon/core.c +0 -0
 	else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
 		numerator = i.freeram;
 	return mult_frac(numerator, 10000, i.totalram);

Could this cause a division by zero if the node is online but memoryless?

For CPU-only compute nodes or nodes where all memory has been hot-removed, node_online(goal->nid) will evaluate to true. When si_meminfo_node() is called for these memoryless nodes, it populates the sysinfo struct with i.totalram = 0.

The mult_frac() macro unconditionally performs integer division and modulo by the third argument, which could trigger a kernel panic when dividing by zero.

Would it be safer to check if i.totalram is zero before returning, or use node_state(goal->nid, N_MEMORY) instead of node_online()?

[ ... ]

Quoted 4 lines
mm/damon/core.c +0 -0
@@ -2261,6 +2265,10 @@ static unsigned long damos_get_node_memcg_used_bp(
 	unsigned long used_pages, numerator;
 	struct sysinfo i;
Quoted 8 lines

mm/damon/core.c static unsigned long damos_get_node_memcg_used_bp( · line 2254

Quoted 5 lines
mm/damon/core.c +4 -0
+	if (goal->nid < 0 || goal->nid >= MAX_NUMNODES ||
+	    !node_online(goal->nid))
+		return 0;
+
 	memcg = mem_cgroup_get_from_id(goal->memcg_id);

The same division by zero risk exists here when mult_frac() is called at the end of this function.

Quoted 8 lines

I think sashiko's comments make sense. I also suggested using node_state(N_MEMORY). What do you think, Josh?

Thanks,
SJ

[...]

Yep, will do