linux-kernel - Re: [PATCH] mm/damon/sysfs: check for online node in target_nid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251210150930.57679-1-sj@kernel.org>
Date: Wed, 10 Dec 2025 07:09:30 -0800
From: SeongJae Park <sj@...nel.org>
To: Swaraj Gaikwad <swarajgaikwad1925@...il.com>
Cc: SeongJae Park <sj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	damon@...ts.linux.dev (open list:DAMON),
	linux-mm@...ck.org (open list:DAMON),
	linux-kernel@...r.kernel.org (open list),
	skhan@...uxfoundation.org,
	david.hunter.linux@...il.com
Subject: Re: [PATCH] mm/damon/sysfs: check for online node in target_nid_store

Hello Swaraj,

On Wed, 10 Dec 2025 11:17:07 +0000 Swaraj Gaikwad <swarajgaikwad1925@...il.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

[...]