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

[SeongJae Park <sj@kernel.org>]

On Fri, 10 Apr 2026 17:40:04 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Sashiko found some issues.

Clarifying what is the following action for the sashiko finding would be
helpful.  For example, if you say "Sashiko found some issues, so I will post a
new versiuon soon", and if I'm busy, I can save my time by saying "Ok, then I
will skip or delaying review of this version and wait for your next review".

Seems you think Sashiko found issues are not worthy to post a new version,
though, so I'm commenting below.

> 
> Forwarding full sashiko review in a reply format with my 
> inline comments below, for sharing details of my view and
> doing followup discussions via mails if needed.
> 
> > # review url: https://sashiko.dev/#/patchset/20260410044259.95877-2-aethernet65535@gmail.com
> > # start of sashiko.dev inline review
> > commit bc048f9325d07815d621e80f3c40408c0210af75
> > Author: Liew Rui Yan <aethernet65535@gmail.com>
> > 
> > mm/damon/lru_sort: validate min_region_size to be power of 2
> > 
> > This commit adds early validation for min_region_sz in
> > damon_lru_sort_apply_parameters(). If the user configures an addr_unit that
> > produces a non-power-of-2 min_region_sz, it immediately returns -EINVAL to
> > avoid setting maybe_corrupted and unexpectedly terminating kdamond.
> > 
> > > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > > index 554559d729760..3fd176ef9d9c1 100644
> > > --- a/mm/damon/lru_sort.c
> > > +++ b/mm/damon/lru_sort.c
> > > @@ -294,6 +294,11 @@ 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)) {
> > > +		err = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +
> > 
> > Is the DAMON sysfs interface also susceptible to this regression?
> > 
> > When a user configures addr_unit via sysfs and writes 'on' to the state file,
> > damon_sysfs_turn_damon_on() builds the context. The min_region_sz is
> > calculated directly from addr_unit without a power-of-two check, and then
> > damon_start() is called, bypassing the damon_commit_ctx() safeguard.
> > 
> > Since the DAMON core uses min_region_sz as an alignment mask in macros like
> > ALIGN() and ALIGN_DOWN(), can this cause memory corruption or infinite loops
> > in functions like damon_fill_regions_holes() if a non-power-of-2 value is
> > provided via sysfs?
> 
> This issue has been discussed before [1]. While it's a valid concern,
> it's outside the scope of this backport patch. SeongJae Park has
> indicated he will work on this.

That's correct.  It's on my todo list.

> 
> > 
> > This patch works around damon_commit_ctx() failing due to !is_power_of_2()
> > by performing the check early, but does damon_commit_ctx() still have a
> > regression leading to unexpected termination if other allocations fail?
> 
> Yes, unexpected termination still exists.
> 
> This patch provides a minimal workaround for the specific case of
> non-power-of-2 'min_region_sz', which is suitable for backport.

I agree.

> 
> > 
> > If damon_commit_ctx() fails midway, for example returning -ENOMEM from
> > damon_commit_schemes(), it leaves dst->maybe_corrupted = true and bails
> > out without rolling back the partial updates.
> > 
> > Even the DAMON sysfs interface, which attempts to mitigate this using a
> > test_ctx dry-run, could experience a time-of-check to time-of-use race
> > where the test allocation succeeds but the final commit allocation fails.
> > 
> > Since these modules operate under memory pressure, if an -ENOMEM occurs
> > during parameter updates, will the running reclaim daemon permanently
> > observe maybe_corrupted = true and unexpectedly terminate itself?
> 
> You're correct. If damon_commit_ctx() fails midway, it leaves
> 'maybe_corrupted=true', causing kdamond to terminate permanently. This
> is a core error handling issue that needs to be fixed, but it's outside
> the scope of this backport patch.

Agreed.  This was unclear to me in previous disucssions, though.  I still agree
it is out of the scope of this patch.  But now I think we need to let users
force-restart.  Adding this to my todo list.

> 
> > 
> > >  	if (!damon_lru_sort_mon_attrs.sample_interval) {
> > >  		err = -EINVAL;
> > >  		goto out;
> > 
> > 
> > # end of sashiko.dev inline review
> > # review url: https://sashiko.dev/#/patchset/20260410044259.95877-2-aethernet65535@gmail.com


Thanks,
SJ

[...]