[PATCH] mm/damon/sysfs: check for online node in target_nid_store

[PATCH] mm/damon/sysfs: check for online node in target_nid_store

[Swaraj Gaikwad]

The 'target_nid_store' function previously accepted any integer value
written to the 'target_nid' sysfs file without validation. This allowed
users to set invalid Node IDs (e.g., -1 or 9999).

This patch adds a check using 'node_online(nid)' to ensure the input
is a valid, online node. If the node is invalid, it returns
-EINVAL. This change also resolves the TODO comment "error handling
for target_nid range".

Test:
Built kernel successfully with CONFIG_DAMON_SYSFS=y. Verified the fix
using a bash script that first creates a DAMON scheme, resets
'target_nid' to a valid value (0), and then attempts to write
invalid values (9999 and -1).

Confirmed that writing 9999 and -1 previously succeeded (incorrectly),
but now fail by rejecting the write and retaining the valid value (0)
as expected.

Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
---
 mm/damon/sysfs-schemes.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 6536f16006c9..b18321e64423 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -2244,11 +2244,18 @@ static ssize_t target_nid_store(struct kobject *kobj,
 	struct damon_sysfs_scheme *scheme = container_of(kobj,
 			struct damon_sysfs_scheme, kobj);
 	int err = 0;
+	int nid;

-	/* TODO: error handling for target_nid range. */
-	err = kstrtoint(buf, 0, &scheme->target_nid);
+	err = kstrtoint(buf, 0, &nid);
+	if (err)
+		return err;

-	return err ? err : count;
+	if (!node_online(nid))
+		return -EINVAL;
+
+	scheme->target_nid = nid;
+
+	return count;
 }

 static void damon_sysfs_scheme_release(struct kobject *kobj)

base-commit: e9a6fb0bcdd7609be6969112f3fbfcce3b1d4a7c
--
2.52.0

Re: [PATCH] mm/damon/sysfs: check for online node in target_nid_store

[SeongJae Park]

Hello Swaraj,

On Wed, 10 Dec 2025 11:17:07 +0000 Swaraj Gaikwad <swarajgaikwad1925@gmail.com> wrote:

> The 'target_nid_store' function previously accepted any integer value
> written to the 'target_nid' sysfs file without validation. This allowed
> users to set invalid Node IDs (e.g., -1 or 9999).

Nice catch!  Actually this was even able to trigger kernel BUG, as commit
7e6c3130690a ("mm/damon/ops-common: ignore migration request to invalid nodes")
explains.  And the commit fixed the issue by avoiding only the kernel BUG,
while still allowing the invalid input.  The intention was to make the
interface as simple as possible unless it causes real issues.  It is for both
maiking the maintenance easy and allowing flexible usages of the interface.  It
is a king of consistent deisgn philosophy of the DAMON sysfs interface.  For
this particular case, the current behavior allows flexible usages regardless of
possible hot [un]plug of NUMA nodes.  IOW, I'd argue this is not a bug but an
intended behavior.

Maybe the commit message should have more clearly explained the intention.  As
the author of the commit, I apology for not making the intention clear.

> 
> This patch adds a check using 'node_online(nid)' to ensure the input
> is a valid, online node. If the node is invalid, it returns
> -EINVAL.

I'm sadly have to say that I'm not very convinced with this change because it
makes a behavioral change.  Making behavioral changes is ok as long as the
changes are obvious improvements.  I don't feel this change is an obvious
improvement for following reasons.  After all, it is making the interface bit
more complicated, in terms of its maintenance.  Seond, and more importantly, it
makes it less flexible in case of possible hot [un]plugging of NUMA nodes.
Suppose the user knows they will hot-plug a NUMA node and wants to set the
target NUMA node before doing the hotplug.

> This change also resolves the TODO comment "error handling
> for target_nid range".

Thank you for mentioning this!  Seems the comment was intended to be addressed
before the code be upstreamed, and I missed it, sorry.  That's also why I'm
saying thank you :)  I think we should just remove the comment.

So, I'd suggest you to send a new version of this patch for such comment
removal only.  Of course, further documentation improvements woudl be nice,
though I wouldn't insist you should do that as a part of the next version of
this patch.  What do you think?

Please let me know if I'm missing something.


Thank,
SJ

[...]

[PATCH v2] mm/damon/sysfs-schemes: Remove outdated TODO in target_nid_store()

[Swaraj Gaikwad]

The TODO comment in target_nid_store() suggested adding range validation
for target_nid. As discussed in [1], the current behavior of accepting
any integer value is intentional. DAMON sysfs aims to remain flexible,
including supporting users who prepare node IDs before future NUMA hotplug
events.

Because this behavior matches the broader design philosophy of the DAMON
sysfs interface, the TODO comment is now misleading. This patch removes the
comment.

No functional changes.

[1] https://lore.kernel.org/lkml/20251210150930.57679-1-sj@kernel.org/

v2:
 - Removed the logic changes (validation) introduced in v1.
 - Updated commit message to reflect that only the comment is being removed.

Suggested-by: SeongJae Park <sj@kernel.org>
Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
---
 mm/damon/sysfs-schemes.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 6536f16006c9..14a17da350ab 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -2245,7 +2245,6 @@ static ssize_t target_nid_store(struct kobject *kobj,
 			struct damon_sysfs_scheme, kobj);
 	int err = 0;

-	/* TODO: error handling for target_nid range. */
 	err = kstrtoint(buf, 0, &scheme->target_nid);

 	return err ? err : count;

base-commit: e9a6fb0bcdd7609be6969112f3fbfcce3b1d4a7c
--
2.52.0

Re: [PATCH v2] mm/damon/sysfs-schemes: Remove outdated TODO in target_nid_store()

[SeongJae Park]

On Thu, 11 Dec 2025 03:07:08 +0000 Swaraj Gaikwad <swarajgaikwad1925@gmail.com> wrote:

> The TODO comment in target_nid_store() suggested adding range validation
> for target_nid. As discussed in [1], the current behavior of accepting
> any integer value is intentional. DAMON sysfs aims to remain flexible,
> including supporting users who prepare node IDs before future NUMA hotplug
> events.
> 
> Because this behavior matches the broader design philosophy of the DAMON
> sysfs interface, the TODO comment is now misleading. This patch removes the
> comment.

Thank you so much for this patch, Swaraj!

You sent this as a reply to v1.  In mm/ and damon/ subsystems, we prefer new
versions of patches to be sent as a new email, rather than a reply to the
previous version.  Please consider doing so next time.

> 
> No functional changes.
> 
> [1] https://lore.kernel.org/lkml/20251210150930.57679-1-sj@kernel.org/

This kind of link is useful for tracking the history.  Thank you for adding
this.

> 
> v2:
>  - Removed the logic changes (validation) introduced in v1.
>  - Updated commit message to reflect that only the comment is being removed.

This kind of change history should be placed under the '---' line [1] below.
Please consider doing so next time.

> 
> Suggested-by: SeongJae Park <sj@kernel.org>
> Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>

Other than the trivial comments above, change looks good to me.

Reviewed-by: SeongJae Park <sj@kernel.org>

If you willing to, please address the comments and send v3.  Because the
comments are for only very trivial things, if you don't or can't do that in
days, I will address those and send it as v3 on my own.

[1] https://docs.kernel.org/process/submitting-patches.html#commentary


Thanks,
SJ

[...]