All of lore.kernel.org
 help / color / mirror / Atom feed
From: SJ Park <sj@kernel.org>
To: sashiko-bot@kernel.org
Cc: SJ Park <sj@kernel.org>, damon@lists.linux.dev
Subject: Re: [PATCH 6.12.y] mm/damon/core: validate ranges in damon_set_regions()
Date: Wed, 29 Jul 2026 20:57:33 -0700	[thread overview]
Message-ID: <20260730035735.140509-1-sj@kernel.org> (raw)
In-Reply-To: <20260730011554.972251F000E9@smtp.kernel.org>

On Thu, 30 Jul 2026 01:15:54 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] The newly added validation check incorrectly uses alignment macros, allowing unaligned zero-length or inverted regions to silently bypass validation.
> - [Medium] The validation check unconditionally rejects valid monitoring ranges ending near the top of the address space due to an integer overflow in the ALIGN macro.
> 
> Pre-existing issues:
> - [High] Alignment rounding combined with a flawed iteration loop creates overlapping monitoring regions from valid, non-overlapping user inputs.
> --
> 
> --- Patch [1]: [PATCH 6.12.y] mm/damon/core: validate ranges in damon_set_regions() ---
> commit b519271ef73c8bbed5092bcf809ea6bc504693b6
> Author: SJ Park <sj@kernel.org>
> Date:   Thu Jul 30 00:56:39 2026 +0000
> 
> mm/damon/core: validate ranges in damon_set_regions()
>     
> This commit attempts to validate regions in damon_set_regions() to reject 
> zero length monitoring target regions, which could previously trigger a 
> divide-by-zero or WARN_ONCE.
> 
> Link: https://lore.kernel.org/20260630035221.146458-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 7911599c37f22..7217bb2ef51ec 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -214,6 +214,12 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
> >  	unsigned int i;
> >  	int err;
> >  
> > +	for (i = 0; i < nr_ranges; i++) {
> > +		if (ALIGN_DOWN(ranges[i].start, DAMON_MIN_REGION) >=
> > +				ALIGN(ranges[i].end, DAMON_MIN_REGION))
> > +			return -EINVAL;
> > +	}
> > +
> 
> [Severity: High]
> Does this validation logic allow unaligned zero-length or inverted regions
> to bypass the check?
> 
> If a user configures an unaligned empty region (e.g., start = 4097, 
> end = 4097) via sysfs, ALIGN_DOWN evaluates to 4096 and ALIGN evaluates to
> 8192. The condition 4096 >= 8192 evaluates to false, which completely 
> bypasses the rejection of the invalid region.

Yes, and no problem.  That's the intended behavior.

> 
> [Severity: Medium]
> Will this check unconditionally reject valid monitoring ranges ending near the
> top of the address space?
> 
> If a caller specifies a monitoring region ending near ULONG_MAX, the
> ALIGN(ranges[i].end, DAMON_MIN_REGION) operation could wrap around to 0.
> This would cause the unsigned comparison ALIGN_DOWN(start) >= 0 to always
> evaluate to true, resulting in a false positive -EINVAL rejection.

Too weired user setup.  I will ignore this unless someone reports different
opinions.

> 
> [Severity: High]
> This is a pre-existing issue, but does alignment rounding combined with the
> iteration loop create overlapping monitoring regions from valid,
> non-overlapping user inputs?
> 
> mm/damon/core.c:damon_set_regions() {
>     ...
>     if (!first) {
>         /* no region intersects with this range */
>         newr = damon_new_region(
>                 ALIGN_DOWN(range->start,
>                     DAMON_MIN_REGION),
>                 ALIGN(range->end, DAMON_MIN_REGION));
>         if (!newr)
>             return -ENOMEM;
>         damon_insert_region(newr, damon_prev_region(r), r, t);
>     } else {
>     ...
> }
> 
> If a user configures adjacent, non-overlapping unaligned ranges (e.g., 
> start = 0, end = 4097 and start = 4098, end = 8192), ALIGN_DOWN and ALIGN 
> expand these to [0, 8192] and [4096, 8192], which overlap.

In the case, the overlapping regions check at the beginning of the function
should returned an error.

> 
> Furthermore, because the loop cursor is not reset, intersection checks are 
> skipped for subsequent ranges once the list end is reached. Could this cause 
> these overlapping regions to be blindly inserted, potentially corrupting 
> core data structures?

So this couldn't happen.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260730005639.108842-1-sj@kernel.org?part=1


Thanks,
SJ

      reply	other threads:[~2026-07-30  3:57 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-29 13:58 FAILED: patch "[PATCH] mm/damon/core: validate ranges in damon_set_regions()" failed to apply to 6.12-stable tree gregkh
2026-07-30  0:56 ` [PATCH 6.12.y] mm/damon/core: validate ranges in damon_set_regions() SJ Park
2026-07-30  1:15   ` sashiko-bot
2026-07-30  3:57     ` SJ Park [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260730035735.140509-1-sj@kernel.org \
    --to=sj@kernel.org \
    --cc=damon@lists.linux.dev \
    --cc=sashiko-bot@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.