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

Josh Law
2026-03-19T15:13:01Z

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