Re: [PATCH] mm/damon/sysfs-schemes: Validate nid usage in nid_show()

[SeongJae Park <sj@kernel.org>]

Hello Swaraj,

On Mon, 20 Oct 2025 11:28:24 +0000 Swaraj Gaikwad <swarajgaikwad1925@gmail.com> wrote:

> The nid_show() function previously returned the node ID (nid) without
> verifying if the goal object of damos_sysfs_quota_goal was using a
> node-based metric. This could lead to incorrect reporting when the
> goal metric was unrelated to node memory.
> 
> This patch introduces a validation step to ensure that nid_show() only
> returns the node ID for valid node-based metrics:
> - DAMOS_QUOTA_NODE_MEM_USED_BP
> - DAMOS_QUOTA_NODE_MEM_FREE_BP
> 
> For other metrics, it returns -EINVAL to prevent misleading information.
> 
> Tested with KUnit:
> - Built kernel with KUnit and DAMON sysfs tests enabled.
> - Executed KUnit tests:
>   ./kunit.py run --kunitconfig ./mm/mm/damon/tests/.kunitconfig
> - All 25 tests passed, including damon_sysfs_test_add_targets.

So nice patch, thank you!  I have a comment below, though.

> 
> Based on commit 3a8660878839 ("Linux 6.18-rc1").

Maybe this is better to be put under the below comment section [1].

> 
> Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
> ---
>  mm/damon/sysfs-schemes.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 6536f16006c9..23a73b94fe53 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -1112,7 +1112,16 @@ static ssize_t nid_show(struct kobject *kobj,
>  	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
>  			damos_sysfs_quota_goal, kobj);
>  
> -	/* todo: return error if the goal is not using nid */
> +	switch (goal->metric) {
> +		case DAMOS_QUOTA_USER_INPUT:
> +		case DAMOS_QUOTA_SOME_MEM_PSI_US:
> +			return -EINVAL;
> +		case DAMOS_QUOTA_NODE_MEM_USED_BP:
> +		case DAMOS_QUOTA_NODE_MEM_FREE_BP:
> +			break;
> +		default:
> +			return -EINVAL;
> +	}
>  
>  	return sysfs_emit(buf, "%d\n", goal->nid);
>  }

According to 'git bisect', I in the past actually added the todo comment.  But,
I of now cannot remember why I in the past did think that's something to do.  I
now think this function is better to be simply show the value the user has set
last time, since users could set the metrics after setting nid.  Other similar
parameters also work in the way, to be wiring-order independent, so that simple
behavior would be better for consistency, too.

I'm sorry if the wrong todo comment was confusing you, Swaraj.  Would you mind
sending a patch removing only the todo comment, without introducing any
behavior change?

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


Thanks,
SJ