linux-kernel - Re: [PATCH 3/6] mm/damon/lru_sort: use param

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250915184306.110418-1-sj@kernel.org>
Date: Mon, 15 Sep 2025 11:43:06 -0700
From: SeongJae Park <sj@...nel.org>
To: Joshua Hahn <joshua.hahnjy@...il.com>
Cc: SeongJae Park <sj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	damon@...ts.linux.dev,
	kernel-team@...a.com,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH 3/6] mm/damon/lru_sort: use param_ctx correctly

On Mon, 15 Sep 2025 08:05:47 -0700 Joshua Hahn <joshua.hahnjy@...il.com> wrote:

> On Sun, 14 Sep 2025 18:58:04 -0700 SeongJae Park <sj@...nel.org> wrote:
> 
> > damon_lru_sort_apply_parameters() allocates a new DAMON context, stages
> > user-specified DAMON parameters on it, and commits to running DAMON
> > context at once, using damon_commit_ctx().  The code is, however,
> > directly updating the monitoring attributes of the running context. This
> > doesn't cause a real user problem but apparently this is an
> > unintentional mistake that can cause code review confusions and future
> > real problems.  Fix the wrong use of the parameter context.
> 
> Hi SJ,
> 
> Thank you for the patch! I am a little bit confused by the behavior in
> damon_lru_sort_apply_parameters. I was hoping that you could help me understand : -)

Thank you for sharing this Joshua!

> In particular, I think that this patch fixes two possible user visible errors.
> 
> My understanding is that we want to make changes to the param_ctx first,
> validate the changes, and commit these changes to the global ctx struct at the
> end. In the middle in the errors, we can abort the operation without committing,
> and ctx will remain unchanged.
> 
> So to me, it does seem like the current code could lead to some visible effects
> from the user's perspective (error-handling case).

damon_set_attrs() has its own parameters validation.  If the validation fails,
it returns an error.  damon_commit_ctx()'s internal validation for damon_attrs
parameters also depend on the validsation of damon_set_attrs().  If the given
parameter is invalid, damon_set_attrs() will return an error without committing
the change, so there should be no user-visible behavioral difference.

> 
> Also, I am a bit confused by how the commit is currently called. We have
> err = damon_commit_ctx(ctx, param_ctx), where the first argument is the
> destination and the second argument is the source. There is a bit of a mismatch
> because in the current code we have the following:
> 
> +------------------------------------------------+
> |              ctx                    param_ctx  |
> +------------------------------------------------+
> | New &damon_lru_sort_mon_attrs                  |
> |                                     New scheme |
> | attrs overwritten to NULL	  <--            |
> | scheme rewritten to new scheme  <--            |
> +------------------------------------------------+
> 
> So in particular, the global ctx will never have the correct attrs pre-patch,
> since it will always be rewritten by param_ctx, which never had its attrs
> initialized.
> 
> I hope this makes sense : -) All of this is just to say that this patch does
> more than just improve review confusions -- I think there at least two errors
> that this fixes for the user. So perhaps a more descriptive commit will be
> helpful in the future, since we are also adding a fixes tag?

Makes sense, thank you for finding this Joshua!  I will send v2 of this patch
with updated commit message!

> 
> Thank you again for the patch, SJ! Feel free to add:
> Reviewed-by: Joshua Hahn <joshua.hahnjy@...il.com>

Thank you!


Thanks,
SJ

[...]