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

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

[Josh Law]

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.

This was unclear from the code alone and led to a separate patch
attempting to reset nr_dests on failure.  Make the intended usage
explicit so future readers do not repeat the confusion.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 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

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@objecting.org> 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@objecting.org>

Other than the above trivial things,

Reviewed-by: SeongJae Park <sj@kernel.org>

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@objecting.org
[2] review url: https://sashiko.dev/#/patchset/20260319071332.114595-1-objecting@objecting.org
[3] https://lore.kernel.org/20260319043309.97966-1-sj@kernel.org


Thanks,
SJ

[...]

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

[SeongJae Park]

On Thu, 19 Mar 2026 07:31:14 -0700 SeongJae Park <sj@kernel.org> wrote:

> Hello Josh,
> 
> On Thu, 19 Mar 2026 07:13:32 +0000 Josh Law <objecting@objecting.org> wrote:
[...] 
> 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?
[...]
> 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.

I just posted the patch:
https://lore.kernel.org/20260319145218.86197-1-sj@kernel.org


Thanks,
SJ

[...]

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

[Josh Law]

On 19 March 2026 15:24:33 GMT, SeongJae Park <sj@kernel.org> wrote:
>On Thu, 19 Mar 2026 07:31:14 -0700 SeongJae Park <sj@kernel.org> wrote:
>
>> Hello Josh,
>> 
>> On Thu, 19 Mar 2026 07:13:32 +0000 Josh Law <objecting@objecting.org> wrote:
>[...] 
>> 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?
>[...]
>> 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.
>
>I just posted the patch:
>https://lore.kernel.org/20260319145218.86197-1-sj@kernel.org
>
>
>Thanks,
>SJ
>
>[...]


Hello, this morning i found some big bugs in sysfs.c, ill post the patche(s) soon