[PATCH] mm/damon: validate min_region_size to be power of 2

[PATCH] mm/damon: validate min_region_size to be power of 2

[Liew Rui Yan]

Problem
=======
damon_commit_ctx() checks if 'min_region_sz' is a power of 2. But if it
is not, kdamond will terminate unexpectedly due to the user's invalid
input.

Solution
========
Add the same check to damon_{lru_sort, reclaim}_apply_parameters(), but
return -EINVAL immediately. This is to prevent user's invalid input
causing kdamond to terminate unexpectedly.

Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---
 mm/damon/lru_sort.c | 3 +++
 mm/damon/reclaim.c  | 3 +++
 2 files changed, 6 insertions(+)

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;
+
 	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;
+
 	if (!damon_reclaim_mon_attrs.aggr_interval) {
 		err = -EINVAL;
 		goto out;
-- 
2.53.0

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

[Liew Rui Yan]

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

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

[Liew Rui Yan]

> > [...]
> > 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?
> 

You are absolutely right. I missed the cleanup path here.

I should have used 'err = -EINVAL; goto out;' to ensure 'param_ctx' is
properly freed, similar to the existing error handling below.

> >  	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?

I will fix this in the next version. Thank you for the catch!

> 
> >  	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

Best regards,
Rui Yan

Re: [PATCH] mm/damon: validate min_region_size to be power of 2

[SeongJae Park]

On Tue, 31 Mar 2026 15:32:31 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Problem
> =======
> damon_commit_ctx() checks if 'min_region_sz' is a power of 2. But if it
> is not, kdamond will terminate unexpectedly due to the user's invalid
> input.
> 
> Solution
> ========
> Add the same check to damon_{lru_sort, reclaim}_apply_parameters(), but
> return -EINVAL immediately. This is to prevent user's invalid input
> causing kdamond to terminate unexpectedly.

Seems sashiko found an issue, and you are preparing the next revision, right?
Assuming so, I will skip code review of this version.
> 
> Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> ---

Weren't there previous versions of this patch?  Could you please add changelog
here, from the next version?


Thanks,
SJ

[...]

Re: [PATCH] mm/damon: validate min_region_size to be power of 2

[Liew Rui Yan]

On Tue, 31 Mar 2026 18:01:56 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Tue, 31 Mar 2026 15:32:31 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> >
> > [...]
> Seems sashiko found an issue, and you are preparing the next revision, right?
> Assuming so, I will skip code review of this version.

Yes, I'm preparing the next revision.

> > 
> > Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> > ---
> 
> Weren't there previous versions of this patch?  Could you please add changelog
> here, from the next version?

Thank you for reminding me. I will add a changelog at the comment area,
and add this to the commit messages.

    This patch is a follow-up to the discussion in [1], where we agreed
    that the validation should be done on min_region_sz rather than
    addr_unit.

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

Best regards,
Rui Yan

Re: [PATCH] mm/damon: validate min_region_size to be power of 2

[SeongJae Park]

On Wed,  1 Apr 2026 16:40:45 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> On Tue, 31 Mar 2026 18:01:56 -0700 SeongJae Park <sj@kernel.org> wrote:
> 
> > On Tue, 31 Mar 2026 15:32:31 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> > >
> > > [...]
> > Seems sashiko found an issue, and you are preparing the next revision, right?
> > Assuming so, I will skip code review of this version.
> 
> Yes, I'm preparing the next revision.
> 
> > > 
> > > Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> > > ---
> > 
> > Weren't there previous versions of this patch?  Could you please add changelog
> > here, from the next version?
> 
> Thank you for reminding me. I will add a changelog at the comment area,
> and add this to the commit messages.
> 
>     This patch is a follow-up to the discussion in [1], where we agreed
>     that the validation should be done on min_region_sz rather than
>     addr_unit.
> 
>     [1] https://lore.kernel.org/20260330233343.4083-1-sj@kernel.org

Sounds good.  I appreciate your continued grateful contributions to DAMON,
Liew.


Thanks,
SJ

[...]