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

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

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