* [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it @ 2025-10-23 6:58 Sunil Khatri 2025-10-23 7:48 ` Arunpravin Paneer Selvam 0 siblings, 1 reply; 6+ messages in thread From: Sunil Khatri @ 2025-10-23 6:58 UTC (permalink / raw) To: Christian König, Felix Kuehling, Alex Deucher, Arunpravin Paneer Selvam, amd-gfx Cc: Sunil Khatri 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; + } + amdgpu_bo_unref(&range->bo); kfree(range); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it 2025-10-23 6:58 [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it Sunil Khatri @ 2025-10-23 7:48 ` Arunpravin Paneer Selvam 2025-10-23 15:30 ` Kuehling, Felix 0 siblings, 1 reply; 6+ messages in thread From: Arunpravin Paneer Selvam @ 2025-10-23 7:48 UTC (permalink / raw) To: Sunil Khatri, Christian König, Felix Kuehling, Alex Deucher, amd-gfx 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; > + } > + > amdgpu_bo_unref(&range->bo); > kfree(range); > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it 2025-10-23 7:48 ` Arunpravin Paneer Selvam @ 2025-10-23 15:30 ` Kuehling, Felix 2025-10-27 14:28 ` Christian König 0 siblings, 1 reply; 6+ messages in thread From: Kuehling, Felix @ 2025-10-23 15:30 UTC (permalink / raw) To: Arunpravin Paneer Selvam, Sunil Khatri, Christian König, Alex Deucher, amd-gfx 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, see https://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. Regards, Felix >> + >> amdgpu_bo_unref(&range->bo); >> kfree(range); >> } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it 2025-10-23 15:30 ` Kuehling, Felix @ 2025-10-27 14:28 ` Christian König 2025-10-27 14:40 ` Khatri, Sunil 0 siblings, 1 reply; 6+ messages in thread From: Christian König @ 2025-10-27 14:28 UTC (permalink / raw) To: Kuehling, Felix, Arunpravin Paneer Selvam, Sunil Khatri, Alex Deucher, amd-gfx 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, see https://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. > > Regards, > Felix > > >>> + >>> amdgpu_bo_unref(&range->bo); >>> kfree(range); >>> } >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it 2025-10-27 14:28 ` Christian König @ 2025-10-27 14:40 ` Khatri, Sunil 2025-10-28 8:53 ` Christian König 0 siblings, 1 reply; 6+ messages in thread From: Khatri, Sunil @ 2025-10-27 14:40 UTC (permalink / raw) To: Christian König, Kuehling, Felix, Arunpravin Paneer Selvam, Sunil Khatri, Alex Deucher, amd-gfx [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it 2025-10-27 14:40 ` Khatri, Sunil @ 2025-10-28 8:53 ` Christian König 0 siblings, 0 replies; 6+ messages in thread From: Christian König @ 2025-10-28 8:53 UTC (permalink / raw) To: Khatri, Sunil, Kuehling, Felix, Arunpravin Paneer Selvam, Sunil Khatri, Alex Deucher, amd-gfx On 10/27/25 15:40, Khatri, Sunil wrote: > > 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, see https://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); That makes absolutely no sense kvfree() should be able to accept a NULL pointer as parameter. So clear NAK to that change. What exactly does the backtrace look like? Regards, Christian. > amdgpu_bo_unref(&range->bo); > kfree(range); > } > > > Regards Sunil Khatri > >>> Regards, >>> Felix >>> >>> >>>>> + >>>>> amdgpu_bo_unref(&range->bo); >>>>> kfree(range); >>>>> } ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-28 8:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-23 6:58 [Patch v1] drm/amdgpu: null check for hmm_pfns ptr before freeing it Sunil Khatri 2025-10-23 7:48 ` Arunpravin Paneer Selvam 2025-10-23 15:30 ` Kuehling, Felix 2025-10-27 14:28 ` Christian König 2025-10-27 14:40 ` Khatri, Sunil 2025-10-28 8:53 ` Christian König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox