Re: [PATCH] mm/damon/core: document damos_commit_dests() failure semantics

[SeongJae Park]

Hello Josh,

On Thu, 19 Mar 2026 07:13:32 +0000 Josh Law <objecting@xxxxxxxxxxxxx> wrote:

> Add a kernel-doc comment to damos_commit_dests() documenting its
> allocation failure contract: on -ENOMEM, the destination structure is
> left in a partially torn-down state that is safe to deallocate via
> damon_destroy_scheme(), but must not be reused for further commits.

Nice patch.  We intentionally do not use kernel-doc comment for static
functions, to not encourage API document readers use the non-public functions.
We still use kernel-doc like comment for static functions, to help developers.
That is, starting the comment with '/*' instead of '/**'.  Maybe 'damon_call()'
is an example.

> 
> This was unclear from the code alone and led to a separate patch
> attempting to reset nr_dests on failure.

Good point.  Adding a link [1] would be nice.

> Make the intended usage
> explicit so future readers do not repeat the confusion.
> 
> Signed-off-by: Josh Law <objecting@xxxxxxxxxxxxx>

Other than the above trivial things,

Reviewed-by: SeongJae Park <sj@xxxxxxxxxx>

I added this patch to my tree, after addressing my above comments.  If you
don't mind, I will post it tomorrow as a v2, for mm.git inclusion.  If you have
different opinions to my comments or you prefer to post v2 on your own, please
feel free to do so :)

> ---
>  mm/damon/core.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index e233eb84a2d5..c884bb31c9b8 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1054,6 +1054,23 @@ static void damos_set_filters_default_reject(struct damos *s)
>  		damos_filters_default_reject(&s->ops_filters);
>  }
>  
> +/**
> + * damos_commit_dests() - Copy migration destinations from @src to @dst.
> + * @dst:	Destination structure to update.
> + * @src:	Source structure to copy from.
> + *
> + * If the number of destinations has changed, the old arrays in @dst are freed
> + * and new ones are allocated.  On success, @dst contains a full copy of
> + * @src's arrays and count.
> + *
> + * On allocation failure, @dst is left in a partially torn-down state: its
> + * arrays may be NULL and @nr_dests may not reflect the actual allocation
> + * sizes.  The structure remains safe to deallocate via damon_destroy_scheme(),
> + * but callers must not reuse @dst for further commits — it should be
> + * discarded.
> + *
> + * Return: 0 on success, -ENOMEM on allocation failure.
> + */
>  static int damos_commit_dests(struct damos_migrate_dests *dst,
>  		struct damos_migrate_dests *src)
>  {
> -- 
> 2.34.1

And Sashiko comment [2].  TL; DR: Good finding.  We aware the issue and working
on the fix.  Nonetheless, that's out of the scope of this patch.

: Does leaving dst in a torn-down state cause a NULL pointer dereference in
: the background kdamond thread?
: 
: If damon_commit_ctx() fails during damos_commit_dests(), the sysfs update
: aborts. However, the broken scheme remains active in kdamond->damon_ctx
: because damon_commit_schemes() does not remove or destroy dst_scheme on
: error.
: 
: When the background thread calls kdamond_apply_schemes() and invokes
: damos_va_migrate_dests_add(), it iterates based on the unmodified nr_dests:
: 
:     for (i = 0; i < dests->nr_dests; i++)
:         weight_total += dests->weight_arr[i];
: 
: Since weight_arr is NULL but nr_dests retains its old non-zero value, will
: this unconditionally dereference the NULL pointer? It appears the previous
: patch attempting to reset nr_dests on failure might be necessary to prevent
: this crash.


Nice catch.  We actually figured out [3] this issue from the discussion on
Josh's previous patch that motivated this one.  And I'm working on it.  Because
the issue is arguably orthogonal to this patch, I think this patch is good to
go.

[1] https://lore.kernel.org/20260318214939.36100-1-objecting@xxxxxxxxxxxxx
[2] review url: https://sashiko.dev/#/patchset/20260319071332.114595-1-objecting@xxxxxxxxxxxxx
[3] https://lore.kernel.org/20260319043309.97966-1-sj@xxxxxxxxxx


Thanks,
SJ

[...]