Re: (sashiko review) [PATCH] mm/damon: validate min_region_size to be power of 2

[Liew Rui Yan <aethernet65535@gmail.com>]

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/20260331073231.30060-1-aethernet65535@gmail.com
# start of sashiko.dev inline review
commit 883916ab04d9fc6b9074dcd36200cd14402cc8fb
Author: Liew Rui Yan <aethernet65535@gmail.com>
mm/damon: validate min_region_size to be power of 2

This patch adds checks to ensure that min_region_sz is a power of 2 in
damon_lru_sort_apply_parameters() and damon_reclaim_apply_parameters().
If the value is not a power of 2, it returns -EINVAL immediately to
prevent kdamond from terminating unexpectedly.

> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 554559d72976..205592194efd 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -294,6 +294,9 @@ static int damon_lru_sort_apply_parameters(void)
>  	param_ctx->addr_unit = addr_unit;
>  	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
>  
> +	if (!is_power_of_2(param_ctx->min_region_sz))
> +		return -EINVAL;
> +

Does this code leak the allocated param_ctx?

Looking earlier in damon_lru_sort_apply_parameters(), param_ctx is allocated
via damon_modules_new_paddr_ctx_target(). The existing error paths below
this check use a goto out; to ensure damon_destroy_ctx(param_ctx) is called.

By returning directly here, does it skip freeing param_ctx?

>  	if (!damon_lru_sort_mon_attrs.sample_interval) {
>  		err = -EINVAL;
>  		goto out;

[ ... ]

> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index 86da14778658..6e29d92670c4 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c
> @@ -204,6 +204,9 @@ static int damon_reclaim_apply_parameters(void)
>  	param_ctx->addr_unit = addr_unit;
>  	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
>  
> +	if (!is_power_of_2(param_ctx->min_region_sz))
> +		return -EINVAL;
> +

Can this result in a similar leak in the reclaim path?

Like the previous file, param_ctx is allocated earlier in this function
and there is an out label that handles cleaning up the context. Could this
be changed to set err = -EINVAL; and goto out; instead of returning directly?

>  	if (!damon_reclaim_mon_attrs.aggr_interval) {
>  		err = -EINVAL;
>  		goto out;


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