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 7/8] mm/damon/core: remove start, end check in damon_set_region_system_rams()
Date: Sun,  5 Jul 2026 09:40:25 -0700	[thread overview]
Message-ID: <20260705164027.101734-1-sj@kernel.org> (raw)
In-Reply-To: <20260705160651.626791F000E9@smtp.kernel.org>

On Sun, 05 Jul 2026 16:06:50 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Removing the explicit `*start > *end` check causes invalid, inverted memory ranges to be inconsistently accepted due to post-alignment bounds checking in `damon_set_regions()`.

No blocker.  Read below for details, please.

> --
> 
> commit f561b43e79bb90540b8053e4d95593cfb96f6d1d
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: remove start, end check in damon_set_region_system_rams()
> 
> This commit removes the explicit start and end validation in
> damon_set_region_system_rams_default(), relying instead on the bounds check
> inside damon_set_regions() to catch invalid ranges.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -3846,9 +3846,6 @@ int damon_set_region_system_rams_default(struct damon_target *t,
> >  {
> >  	struct damon_addr_range addr_range;
> >  
> > -	if (*start > *end)
> > -		return -EINVAL;
> > -
> 
> [Severity: Medium]
> Does removing this check allow inverted ranges to bypass validation?
> 
> Looking at damon_set_regions() in mm/damon/core.c, the fallback check
> evaluates the values post-alignment:
> 
> mm/damon/core.c:damon_set_regions() {
> 	for (i = 0; i < nr_ranges; i++) {
> 		unsigned long start, end;
> 
> 		start = ALIGN_DOWN(ranges[i].start, min_region_sz);
> 		end = ALIGN(ranges[i].end, min_region_sz);
> 		if (start >= end)
> 			return -EINVAL;
> 	...
> }
> 
> If an administrator configures an unaligned, inverted memory range
> (for example, start=8191, end=4097), the post-alignment check might still
> evaluate to ALIGN_DOWN(start) < ALIGN(end) and accept the invalid inputs.

Yes, that's how it works, and I find no problem here.  No bad consequences like
kernel panic happenns.  It it might be confusing and better to improve for user
experience.  However, I don't think this corner case experience deserves the
additional complexity.  I want to keep the validation simple and minimum.

So I will keep this as is unless others chime in.

> 
> >  	if (!*start && !*end &&
> >  		!damon_find_system_rams_range(start, end, addr_unit))
> >  		return -EINVAL;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260705155600.96555-1-sj@kernel.org?part=7

Thankss,
SJ

  reply	other threads:[~2026-07-05 16:40 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-05 15:55 [PATCH 0/8] mm/damon: validate all parameters in the core SJ Park
2026-07-05 15:55 ` [PATCH 1/8] mm/damon/core: safely validate src on damon_commit_ctx() SJ Park
2026-07-05 15:55 ` [PATCH 2/8] mm/damon/core: do parameter testing commit on damon_start() SJ Park
2026-07-05 15:55 ` [PATCH 3/8] mm/damon/sysfs: remove duplicated commit input validity check SJ Park
2026-07-05 15:55 ` [PATCH 4/8] mm/damon/reclaim: remove duplicated min_region_sz power of 2 check SJ Park
2026-07-05 16:12   ` sashiko-bot
2026-07-05 16:36     ` SJ Park
2026-07-05 15:55 ` [PATCH 5/8] mm/damon/lru_sort: remove duplicated min_region_sz power_of_2() check SJ Park
2026-07-05 15:55 ` [PATCH 6/8] mm/damon: document region size validation in damon_set_regions() SJ Park
2026-07-05 15:55 ` [PATCH 7/8] mm/damon/core: remove start, end check in damon_set_region_system_rams() SJ Park
2026-07-05 16:06   ` sashiko-bot
2026-07-05 16:40     ` SJ Park [this message]
2026-07-05 15:55 ` [PATCH 8/8] mm/damon/sysfs: remove region size validation SJ Park
2026-07-05 16:05   ` sashiko-bot
2026-07-05 16:41     ` SJ Park
2026-07-05 16:46 ` [PATCH 0/8] mm/damon: validate all parameters in the core SJ Park
2026-07-05 19:16 ` Andrew Morton
2026-07-05 19:43   ` SJ Park

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=20260705164027.101734-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.