[PATCH] mm/damon/core: reset nr_dests on allocation failure in damos_commit_dests()

[PATCH] mm/damon/core: reset nr_dests on allocation failure in damos_commit_dests()

[Josh Law]

damos_commit_dests() frees the old node_id_arr and weight_arr before
reallocating.  If kmalloc_array() fails, the function returns -ENOMEM but
leaves dst->nr_dests at its previous value.  A subsequent call with the
same nr_dests will skip the reallocation (the sizes match), and the loop
at the end will dereference the now-NULL array pointers.

Fix this by resetting dst->nr_dests to 0 immediately after freeing the
old arrays, so any later call always enters the reallocation path.

Fixes: cbc4eea4ffb5 ("mm/damon/core: commit damos->migrate_dests")
Signed-off-by: Josh Law <objecting@objecting.org>
---
 mm/damon/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 7f74982535ac..e233eb84a2d5 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1060,6 +1060,7 @@ static int damos_commit_dests(struct damos_migrate_dests *dst,
 	if (dst->nr_dests != src->nr_dests) {
 		kfree(dst->node_id_arr);
 		kfree(dst->weight_arr);
+		dst->nr_dests = 0;
 
 		dst->node_id_arr = kmalloc_array(src->nr_dests,
 			sizeof(*dst->node_id_arr), GFP_KERNEL);
-- 
2.34.1

Re: [PATCH] mm/damon/core: reset nr_dests on allocation failure in damos_commit_dests()

[SeongJae Park]

Hello Josh,

On Wed, 18 Mar 2026 21:49:39 +0000 Josh Law <objecting@objecting.org> wrote:

> damos_commit_dests() frees the old node_id_arr and weight_arr before
> reallocating.  If kmalloc_array() fails, the function returns -ENOMEM but
> leaves dst->nr_dests at its previous value.  A subsequent call with the
> same nr_dests will skip the reallocation (the sizes match), and the loop
> at the end will dereference the now-NULL array pointers.

Nice catch.  But, this is a sort of intended behavior.

The idea behind the code is that, if the function fails, the caller will
not resue 'dst' but discard it.  Hence the function is only ensuring the 'dst'
after the failure can be deallocated using the deallocation helper function
like 'damon_destroy_scheme()'.  For this, the function is setting weight_arr as
NULL in the allocation failure.

> 
> Fix this by resetting dst->nr_dests to 0 immediately after freeing the
> old arrays, so any later call always enters the reallocation path.
> 
> Fixes: cbc4eea4ffb5 ("mm/damon/core: commit damos->migrate_dests")
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---
>  mm/damon/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 7f74982535ac..e233eb84a2d5 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1060,6 +1060,7 @@ static int damos_commit_dests(struct damos_migrate_dests *dst,
>  	if (dst->nr_dests != src->nr_dests) {
>  		kfree(dst->node_id_arr);
>  		kfree(dst->weight_arr);
> +		dst->nr_dests = 0;
>  
>  		dst->node_id_arr = kmalloc_array(src->nr_dests,
>  			sizeof(*dst->node_id_arr), GFP_KERNEL);

Someone (including a part of myself) could argue anyway initializing the field
is better to do, for code readability and completeness of the data structure.
But I'd argue that might only encourage calllers to reuse 'dst' after the
failure.  Also, the 0 nr_dests could still meaning something incorrect, if the
first kmalloc_array() for node_id_arr success but the following kmalloc_array()
for weight_arr failed.  In the case, nr_dests is zero, but the size of
node_id_arr is not zero.

I think the intention behind the code is not well documented and that might
confused you.  Sorry if that was the case.  I think this could better be
documented by adding comments for the function.  The single line comment in the
function body was for the purpose, but having more detailed comments at the top
of the function may be better.  If you'd like to send such documentation,
please do so.  If not, I will do that.  Whatever is your preference, thank you
for finding and sharing this room to improve!

... And, this patch helped me finding something actually broken.  As I
mentioned above, callers of damos_commit_dests() are assumed to discard the
'dst' when the function failed.  And the only caller, sysfs.c, does so, except
for the final commit to the running context (kdmond->damon_ctx).  It can result
in DAMON running with the incorrect data structure, doing NULL dereference.
Similar issue might exist for DAMON_RECLAIM and DAMON_LRU_SORT.  Because those
modules use only limited parameters, there might be not.  I will double check
and make a fix soon.  Again, thank you for helping me finding this issue, Josh!


Thanks,
SJ

Re: [PATCH] mm/damon/core: reset nr_dests on allocation failure in damos_commit_dests()

[Josh Law]

On 19 March 2026 04:33:09 GMT, SeongJae Park <sj@kernel.org> wrote:
>Hello Josh,
>
>On Wed, 18 Mar 2026 21:49:39 +0000 Josh Law <objecting@objecting.org> wrote:
>
>> damos_commit_dests() frees the old node_id_arr and weight_arr before
>> reallocating.  If kmalloc_array() fails, the function returns -ENOMEM but
>> leaves dst->nr_dests at its previous value.  A subsequent call with the
>> same nr_dests will skip the reallocation (the sizes match), and the loop
>> at the end will dereference the now-NULL array pointers.
>
>Nice catch.  But, this is a sort of intended behavior.
>
>The idea behind the code is that, if the function fails, the caller will
>not resue 'dst' but discard it.  Hence the function is only ensuring the 'dst'
>after the failure can be deallocated using the deallocation helper function
>like 'damon_destroy_scheme()'.  For this, the function is setting weight_arr as
>NULL in the allocation failure.
>
>> 
>> Fix this by resetting dst->nr_dests to 0 immediately after freeing the
>> old arrays, so any later call always enters the reallocation path.
>> 
>> Fixes: cbc4eea4ffb5 ("mm/damon/core: commit damos->migrate_dests")
>> Signed-off-by: Josh Law <objecting@objecting.org>
>> ---
>>  mm/damon/core.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index 7f74982535ac..e233eb84a2d5 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -1060,6 +1060,7 @@ static int damos_commit_dests(struct damos_migrate_dests *dst,
>>  	if (dst->nr_dests != src->nr_dests) {
>>  		kfree(dst->node_id_arr);
>>  		kfree(dst->weight_arr);
>> +		dst->nr_dests = 0;
>>  
>>  		dst->node_id_arr = kmalloc_array(src->nr_dests,
>>  			sizeof(*dst->node_id_arr), GFP_KERNEL);
>
>Someone (including a part of myself) could argue anyway initializing the field
>is better to do, for code readability and completeness of the data structure.
>But I'd argue that might only encourage calllers to reuse 'dst' after the
>failure.  Also, the 0 nr_dests could still meaning something incorrect, if the
>first kmalloc_array() for node_id_arr success but the following kmalloc_array()
>for weight_arr failed.  In the case, nr_dests is zero, but the size of
>node_id_arr is not zero.
>
>I think the intention behind the code is not well documented and that might
>confused you.  Sorry if that was the case.  I think this could better be
>documented by adding comments for the function.  The single line comment in the
>function body was for the purpose, but having more detailed comments at the top
>of the function may be better.  If you'd like to send such documentation,
>please do so.  If not, I will do that.  Whatever is your preference, thank you
>for finding and sharing this room to improve!
>
>... And, this patch helped me finding something actually broken.  As I
>mentioned above, callers of damos_commit_dests() are assumed to discard the
>'dst' when the function failed.  And the only caller, sysfs.c, does so, except
>for the final commit to the running context (kdmond->damon_ctx).  It can result
>in DAMON running with the incorrect data structure, doing NULL dereference.
>Similar issue might exist for DAMON_RECLAIM and DAMON_LRU_SORT.  Because those
>modules use only limited parameters, there might be not.  I will double check
>and make a fix soon.  Again, thank you for helping me finding this issue, Josh!
>
>
>Thanks,
>SJ


Well, I guess hardening this patch is useful for then..


V/R


Josh Law

Re: [PATCH] mm/damon/core: reset nr_dests on allocation failure in damos_commit_dests()

[SeongJae Park]

On Thu, 19 Mar 2026 07:07:18 +0000 Josh Law <objecting@objecting.org> wrote:

> 
> 
> On 19 March 2026 04:33:09 GMT, SeongJae Park <sj@kernel.org> wrote:
> >Hello Josh,
> >
> >On Wed, 18 Mar 2026 21:49:39 +0000 Josh Law <objecting@objecting.org> wrote:
> >
> >> damos_commit_dests() frees the old node_id_arr and weight_arr before
> >> reallocating.  If kmalloc_array() fails, the function returns -ENOMEM but
> >> leaves dst->nr_dests at its previous value.  A subsequent call with the
> >> same nr_dests will skip the reallocation (the sizes match), and the loop
> >> at the end will dereference the now-NULL array pointers.
> >
> >Nice catch.  But, this is a sort of intended behavior.
> >
> >The idea behind the code is that, if the function fails, the caller will
> >not resue 'dst' but discard it.  Hence the function is only ensuring the 'dst'
> >after the failure can be deallocated using the deallocation helper function
> >like 'damon_destroy_scheme()'.  For this, the function is setting weight_arr as
> >NULL in the allocation failure.
> >
> >> 
> >> Fix this by resetting dst->nr_dests to 0 immediately after freeing the
> >> old arrays, so any later call always enters the reallocation path.
> >> 
> >> Fixes: cbc4eea4ffb5 ("mm/damon/core: commit damos->migrate_dests")
> >> Signed-off-by: Josh Law <objecting@objecting.org>
> >> ---
> >>  mm/damon/core.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/mm/damon/core.c b/mm/damon/core.c
> >> index 7f74982535ac..e233eb84a2d5 100644
> >> --- a/mm/damon/core.c
> >> +++ b/mm/damon/core.c
> >> @@ -1060,6 +1060,7 @@ static int damos_commit_dests(struct damos_migrate_dests *dst,
> >>  	if (dst->nr_dests != src->nr_dests) {
> >>  		kfree(dst->node_id_arr);
> >>  		kfree(dst->weight_arr);
> >> +		dst->nr_dests = 0;
> >>  
> >>  		dst->node_id_arr = kmalloc_array(src->nr_dests,
> >>  			sizeof(*dst->node_id_arr), GFP_KERNEL);
> >
> >Someone (including a part of myself) could argue anyway initializing the field
> >is better to do, for code readability and completeness of the data structure.
> >But I'd argue that might only encourage calllers to reuse 'dst' after the
> >failure.  Also, the 0 nr_dests could still meaning something incorrect, if the
> >first kmalloc_array() for node_id_arr success but the following kmalloc_array()
> >for weight_arr failed.  In the case, nr_dests is zero, but the size of
> >node_id_arr is not zero.
> >
> >I think the intention behind the code is not well documented and that might
> >confused you.  Sorry if that was the case.  I think this could better be
> >documented by adding comments for the function.  The single line comment in the
> >function body was for the purpose, but having more detailed comments at the top
> >of the function may be better.  If you'd like to send such documentation,
> >please do so.  If not, I will do that.  Whatever is your preference, thank you
> >for finding and sharing this room to improve!
> >
> >... And, this patch helped me finding something actually broken.  As I
> >mentioned above, callers of damos_commit_dests() are assumed to discard the
> >'dst' when the function failed.  And the only caller, sysfs.c, does so, except
> >for the final commit to the running context (kdmond->damon_ctx).  It can result
> >in DAMON running with the incorrect data structure, doing NULL dereference.
> >Similar issue might exist for DAMON_RECLAIM and DAMON_LRU_SORT.  Because those
> >modules use only limited parameters, there might be not.  I will double check
> >and make a fix soon.  Again, thank you for helping me finding this issue, Josh!
> >
> >
> >Thanks,
> >SJ
> 
> 
> Well, I guess hardening this patch is useful for then..

Agreed.  Maybe adding another sanity check (e.g., WARN_ON(dst->nr_dests &&
(!dst->weight_arr || !dst->node_id_arr), "foo")) under DAMON_DEBUG_SANITY might
make sense.


Thanks,
SJ

[...]

Re: [PATCH] mm/damon/core: reset nr_dests on allocation failure in damos_commit_dests()

[Josh Law]

On 19 March 2026 14:34:36 GMT, SeongJae Park <sj@kernel.org> wrote:
>On Thu, 19 Mar 2026 07:07:18 +0000 Josh Law <objecting@objecting.org> wrote:
>
>> 
>> 
>> On 19 March 2026 04:33:09 GMT, SeongJae Park <sj@kernel.org> wrote:
>> >Hello Josh,
>> >
>> >On Wed, 18 Mar 2026 21:49:39 +0000 Josh Law <objecting@objecting.org> wrote:
>> >
>> >> damos_commit_dests() frees the old node_id_arr and weight_arr before
>> >> reallocating.  If kmalloc_array() fails, the function returns -ENOMEM but
>> >> leaves dst->nr_dests at its previous value.  A subsequent call with the
>> >> same nr_dests will skip the reallocation (the sizes match), and the loop
>> >> at the end will dereference the now-NULL array pointers.
>> >
>> >Nice catch.  But, this is a sort of intended behavior.
>> >
>> >The idea behind the code is that, if the function fails, the caller will
>> >not resue 'dst' but discard it.  Hence the function is only ensuring the 'dst'
>> >after the failure can be deallocated using the deallocation helper function
>> >like 'damon_destroy_scheme()'.  For this, the function is setting weight_arr as
>> >NULL in the allocation failure.
>> >
>> >> 
>> >> Fix this by resetting dst->nr_dests to 0 immediately after freeing the
>> >> old arrays, so any later call always enters the reallocation path.
>> >> 
>> >> Fixes: cbc4eea4ffb5 ("mm/damon/core: commit damos->migrate_dests")
>> >> Signed-off-by: Josh Law <objecting@objecting.org>
>> >> ---
>> >>  mm/damon/core.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >> 
>> >> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> >> index 7f74982535ac..e233eb84a2d5 100644
>> >> --- a/mm/damon/core.c
>> >> +++ b/mm/damon/core.c
>> >> @@ -1060,6 +1060,7 @@ static int damos_commit_dests(struct damos_migrate_dests *dst,
>> >>  	if (dst->nr_dests != src->nr_dests) {
>> >>  		kfree(dst->node_id_arr);
>> >>  		kfree(dst->weight_arr);
>> >> +		dst->nr_dests = 0;
>> >>  
>> >>  		dst->node_id_arr = kmalloc_array(src->nr_dests,
>> >>  			sizeof(*dst->node_id_arr), GFP_KERNEL);
>> >
>> >Someone (including a part of myself) could argue anyway initializing the field
>> >is better to do, for code readability and completeness of the data structure.
>> >But I'd argue that might only encourage calllers to reuse 'dst' after the
>> >failure.  Also, the 0 nr_dests could still meaning something incorrect, if the
>> >first kmalloc_array() for node_id_arr success but the following kmalloc_array()
>> >for weight_arr failed.  In the case, nr_dests is zero, but the size of
>> >node_id_arr is not zero.
>> >
>> >I think the intention behind the code is not well documented and that might
>> >confused you.  Sorry if that was the case.  I think this could better be
>> >documented by adding comments for the function.  The single line comment in the
>> >function body was for the purpose, but having more detailed comments at the top
>> >of the function may be better.  If you'd like to send such documentation,
>> >please do so.  If not, I will do that.  Whatever is your preference, thank you
>> >for finding and sharing this room to improve!
>> >
>> >... And, this patch helped me finding something actually broken.  As I
>> >mentioned above, callers of damos_commit_dests() are assumed to discard the
>> >'dst' when the function failed.  And the only caller, sysfs.c, does so, except
>> >for the final commit to the running context (kdmond->damon_ctx).  It can result
>> >in DAMON running with the incorrect data structure, doing NULL dereference.
>> >Similar issue might exist for DAMON_RECLAIM and DAMON_LRU_SORT.  Because those
>> >modules use only limited parameters, there might be not.  I will double check
>> >and make a fix soon.  Again, thank you for helping me finding this issue, Josh!
>> >
>> >
>> >Thanks,
>> >SJ
>> 
>> 
>> Well, I guess hardening this patch is useful for then..
>
>Agreed.  Maybe adding another sanity check (e.g., WARN_ON(dst->nr_dests &&
>(!dst->weight_arr || !dst->node_id_arr), "foo")) under DAMON_DEBUG_SANITY might
>make sense.
>
>
>Thanks,
>SJ
>
>[...]


Maybe merge it as-is, because warn crashes the kernel anyway and the patch mitigates it.

V/R

Josh Law

Re: [PATCH] mm/damon/core: reset nr_dests on allocation failure in damos_commit_dests()

[Josh Law]

On 19 March 2026 04:33:09 GMT, SeongJae Park <sj@kernel.org> wrote:
>Hello Josh,
>
>On Wed, 18 Mar 2026 21:49:39 +0000 Josh Law <objecting@objecting.org> wrote:
>
>> damos_commit_dests() frees the old node_id_arr and weight_arr before
>> reallocating.  If kmalloc_array() fails, the function returns -ENOMEM but
>> leaves dst->nr_dests at its previous value.  A subsequent call with the
>> same nr_dests will skip the reallocation (the sizes match), and the loop
>> at the end will dereference the now-NULL array pointers.
>
>Nice catch.  But, this is a sort of intended behavior.
>
>The idea behind the code is that, if the function fails, the caller will
>not resue 'dst' but discard it.  Hence the function is only ensuring the 'dst'
>after the failure can be deallocated using the deallocation helper function
>like 'damon_destroy_scheme()'.  For this, the function is setting weight_arr as
>NULL in the allocation failure.
>
>> 
>> Fix this by resetting dst->nr_dests to 0 immediately after freeing the
>> old arrays, so any later call always enters the reallocation path.
>> 
>> Fixes: cbc4eea4ffb5 ("mm/damon/core: commit damos->migrate_dests")
>> Signed-off-by: Josh Law <objecting@objecting.org>
>> ---
>>  mm/damon/core.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index 7f74982535ac..e233eb84a2d5 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -1060,6 +1060,7 @@ static int damos_commit_dests(struct damos_migrate_dests *dst,
>>  	if (dst->nr_dests != src->nr_dests) {
>>  		kfree(dst->node_id_arr);
>>  		kfree(dst->weight_arr);
>> +		dst->nr_dests = 0;
>>  
>>  		dst->node_id_arr = kmalloc_array(src->nr_dests,
>>  			sizeof(*dst->node_id_arr), GFP_KERNEL);
>
>Someone (including a part of myself) could argue anyway initializing the field
>is better to do, for code readability and completeness of the data structure.
>But I'd argue that might only encourage calllers to reuse 'dst' after the
>failure.  Also, the 0 nr_dests could still meaning something incorrect, if the
>first kmalloc_array() for node_id_arr success but the following kmalloc_array()
>for weight_arr failed.  In the case, nr_dests is zero, but the size of
>node_id_arr is not zero.
>
>I think the intention behind the code is not well documented and that might
>confused you.  Sorry if that was the case.  I think this could better be
>documented by adding comments for the function.  The single line comment in the
>function body was for the purpose, but having more detailed comments at the top
>of the function may be better.  If you'd like to send such documentation,
>please do so.  If not, I will do that.  Whatever is your preference, thank you
>for finding and sharing this room to improve!
>
>... And, this patch helped me finding something actually broken.  As I
>mentioned above, callers of damos_commit_dests() are assumed to discard the
>'dst' when the function failed.  And the only caller, sysfs.c, does so, except
>for the final commit to the running context (kdmond->damon_ctx).  It can result
>in DAMON running with the incorrect data structure, doing NULL dereference.
>Similar issue might exist for DAMON_RECLAIM and DAMON_LRU_SORT.  Because those
>modules use only limited parameters, there might be not.  I will double check
>and make a fix soon.  Again, thank you for helping me finding this issue, Josh!
>
>
>Thanks,
>SJ


Alrighty, I submitted a patch for the kdocs comment, this makes a lot more sense now, and thanks for being super nice!

V/R


Josh Law