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: <20250915150548.1486984-1-joshua.hahnjy@gmail.com>
Date: Mon, 15 Sep 2025 08:05:47 -0700
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: SeongJae Park <sj@...nel.org>
Cc: 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 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 : -)
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).

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?

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

Have a great day!
Joshua

> Fixes: a30969436428 ("mm/damon/lru_sort: use damon_commit_ctx()")
> Signed-off-by: SeongJae Park <sj@...nel.org>
> ---
>  mm/damon/lru_sort.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 14d31009c09e..ab6173a646bd 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -219,7 +219,7 @@ static int damon_lru_sort_apply_parameters(void)
>  		goto out;
>  	}
>  
> -	err = damon_set_attrs(ctx, &damon_lru_sort_mon_attrs);
> +	err = damon_set_attrs(param_ctx, &damon_lru_sort_mon_attrs);
>  	if (err)
>  		goto out;
>  
> -- 
> 2.39.5

Sent using hkml (https://github.com/sjp38/hackermail)