Re: [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it

["Khatri, Sunil" <sukhatri@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 2888 bytes --]


On 27-10-2025 07:58 pm, Christian König wrote:
>
> On 10/23/25 17:30, Kuehling, Felix wrote:
>> On 2025-10-23 03:48, Arunpravin Paneer Selvam wrote:
>>> Acked-by: Arunpravin Paneer Selvam<Arunpravin.PaneerSelvam@amd.com>
>>>
>>> Regards,
>>> Arun.
>>> On 10/23/2025 12:28 PM, Sunil Khatri wrote:
>>>> Due to low memory or when num of pages is too big to be
>>>> accomodated, allocation could fail for pfn's.
>>>>
>>>> Chekc hmm_pfns for NULL before calling the kvfree for the it.
>>>>
>>>> Signed-off-by: Sunil Khatri<sunil.khatri@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>> index d6f903a2d573..6ac206e2bc46 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>> @@ -286,7 +286,11 @@ void amdgpu_hmm_range_free(struct amdgpu_hmm_range *range)
>>>>        if (!range)
>>>>            return;
>>>>    -    kvfree(range->hmm_range.hmm_pfns);
>>>> +    if (range->hmm_range.hmm_pfns) {
>>>> +        kvfree(range->hmm_range.hmm_pfns);
>>>> +        range->hmm_range.hmm_pfns = NULL;
>>>> +    }
>> NULL-checks before kfree and friends are unnecessary. There are actually static checkers that complain about such unnecessary NULL-checks. For example, seehttps://lkml.org/lkml/2024/8/11/168.
>>
>> The same is also true for the standard libc free in usermode:https://stackoverflow.com/questions/1912325/checking-for-null-before-calling-free.
>>
>> Finally, setting range->hmm_range.hmm_pfns = NULL is also unnecessary because you're about to free the whole range structure anyway.
> Agree completely with Felix.
>
> Sunil why do you think that this is necessary and blocking KFD for some reason?
>
> Regards,
> Christian.

KFD side reported the error of NULL dereference

pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL); //fails if the 
size is too big.

Now when we free the memory in function amdgpu_hmm_range_free and try to 
do a kvfree of the range->hmm_range.hmm_pfns which is NULL and we were 
seeing the NULL dereference.
So i added a check to check for the memory to be valid ptr first before 
calling kvfree.

This actually fixed the issue but i do agree that *"setting 
range->hmm_range.hmm_pfns = NULL could be avoided and that why i did not 
added that check in the final patch that i merged" This is the final 
code after this merge.*

voidamdgpu_hmm_range_free(structamdgpu_hmm_range*range)
{
if(!range)
return;
if(range->hmm_range.hmm_pfns)
kvfree(range->hmm_range.hmm_pfns);
amdgpu_bo_unref(&range->bo);
kfree(range);
}


Regards Sunil Khatri

>> Regards,
>>    Felix
>>
>>
>>>> +
>>>>        amdgpu_bo_unref(&range->bo);
>>>>        kfree(range);
>>>>    }

[-- Attachment #2: Type: text/html, Size: 7303 bytes --]