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

[Josh Law <objecting@objecting.org>]

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