[PATCH 1/3] drm/amdgpu: svm check hmm range kzalloc return NULL

[PATCH 1/3] drm/amdgpu: svm check hmm range kzalloc return NULL

[Philip Yang]

Add hmm_range kzalloc return NULL error check. In case the get_pages
return failed, free and set hmm_range to NULL, to avoid double free in
get_pages_done.

Fixes: 29e6f5716115 ("drm/amdgpu: use user provided hmm_range buffer in amdgpu_ttm_tt_get_user_pages")
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 8c3787b00f36..e8a15751c125 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1736,15 +1736,20 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
 				continue;
 			}
 
-			WRITE_ONCE(p->svms.faulting_task, current);
 			hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
-			r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, npages,
-						       readonly, owner,
-						       hmm_range);
-			WRITE_ONCE(p->svms.faulting_task, NULL);
-			if (r) {
-				kfree(hmm_range);
-				pr_debug("failed %d to get svm range pages\n", r);
+			if (unlikely(!hmm_range)) {
+				r = -ENOMEM;
+			} else {
+				WRITE_ONCE(p->svms.faulting_task, current);
+				r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, npages,
+							       readonly, owner,
+							       hmm_range);
+				WRITE_ONCE(p->svms.faulting_task, NULL);
+				if (r) {
+					kfree(hmm_range);
+					hmm_range = NULL;
+					pr_debug("failed %d to get svm range pages\n", r);
+				}
 			}
 		} else {
 			r = -EFAULT;
-- 
2.49.0

[PATCH 2/3] drm/amdkfd: svm unmap use page aligned address

[Philip Yang]

svm_range_unmap_from_gpus uses page aligned start, end address, the end
address is inclusive.

Fixes: 38c55f6719f7 ("drm/amdkfd: Handle lack of READ permissions in SVM mapping")
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index e8a15751c125..742c28833650 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1723,8 +1723,8 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
 				svm_range_lock(prange);
 				if (vma->vm_flags & VM_WRITE)
 					pr_debug("VM_WRITE without VM_READ is not supported");
-				s = max(start, prange->start);
-				e = min(end, prange->last);
+				s = max(start >> PAGE_SHIFT, prange->start);
+				e = min((end - 1) >> PAGE_SHIFT, prange->last);
 				if (e >= s)
 					r = svm_range_unmap_from_gpus(prange, s, e,
 						       KFD_SVM_UNMAP_TRIGGER_UNMAP_FROM_CPU);
-- 
2.49.0

[PATCH 3/3] drm/amdkfd: Don't stuck in svm restore worker

[Philip Yang]

If vma is not found, the application has freed the memory using madvise
MADV_FREE, but driver don't receive the unmap from CPU MMU notifier
callback, the memory is still mapped on GPUs. svm restore work will
schedule the work to retry forever. Then user queues not resumed and
cause application hangs to wait for queue finish.

svm restore work should unmap the memory range from GPUs then resume
queues. If GPU page fault happens on the unmapped address, it is
application use-after-free bug.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 78 ++++++++++++++--------------
 1 file changed, 40 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 742c28833650..608a25c6c865 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1708,51 +1708,53 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
 		bool readonly;
 
 		vma = vma_lookup(mm, addr);
-		if (vma) {
-			readonly = !(vma->vm_flags & VM_WRITE);
+		next = vma ? min(vma->vm_end, end) : end;
 
-			next = min(vma->vm_end, end);
-			npages = (next - addr) >> PAGE_SHIFT;
+		if (!vma || !(vma->vm_flags & VM_READ)) {
 			/* HMM requires at least READ permissions. If provided with PROT_NONE,
 			 * unmap the memory. If it's not already mapped, this is a no-op
 			 * If PROT_WRITE is provided without READ, warn first then unmap
+			 * If vma is not found, addr is invalid, unmap from GPUs
 			 */
-			if (!(vma->vm_flags & VM_READ)) {
-				unsigned long e, s;
-
-				svm_range_lock(prange);
-				if (vma->vm_flags & VM_WRITE)
-					pr_debug("VM_WRITE without VM_READ is not supported");
-				s = max(start >> PAGE_SHIFT, prange->start);
-				e = min((end - 1) >> PAGE_SHIFT, prange->last);
-				if (e >= s)
-					r = svm_range_unmap_from_gpus(prange, s, e,
-						       KFD_SVM_UNMAP_TRIGGER_UNMAP_FROM_CPU);
-				svm_range_unlock(prange);
-				/* If unmap returns non-zero, we'll bail on the next for loop
-				 * iteration, so just leave r and continue
-				 */
-				addr = next;
-				continue;
-			}
+			unsigned long e, s;
+
+			svm_range_lock(prange);
+
+			if (!vma)
+				pr_debug("vma not found\n");
+			else if (vma->vm_flags & VM_WRITE)
+				pr_debug("VM_WRITE without VM_READ is not supported");
+
+			s = max(start >> PAGE_SHIFT, prange->start);
+			e = min((end - 1) >> PAGE_SHIFT, prange->last);
+			if (e >= s)
+				r = svm_range_unmap_from_gpus(prange, s, e,
+					       KFD_SVM_UNMAP_TRIGGER_UNMAP_FROM_CPU);
+			svm_range_unlock(prange);
+			/* If unmap returns non-zero, we'll bail on the next for loop
+			 * iteration, so just leave r and continue
+			 */
+			addr = next;
+			continue;
+		}
 
-			hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
-			if (unlikely(!hmm_range)) {
-				r = -ENOMEM;
-			} else {
-				WRITE_ONCE(p->svms.faulting_task, current);
-				r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, npages,
-							       readonly, owner,
-							       hmm_range);
-				WRITE_ONCE(p->svms.faulting_task, NULL);
-				if (r) {
-					kfree(hmm_range);
-					hmm_range = NULL;
-					pr_debug("failed %d to get svm range pages\n", r);
-				}
-			}
+		readonly = !(vma->vm_flags & VM_WRITE);
+		npages = (next - addr) >> PAGE_SHIFT;
+
+		hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
+		if (unlikely(!hmm_range)) {
+			r = -ENOMEM;
 		} else {
-			r = -EFAULT;
+			WRITE_ONCE(p->svms.faulting_task, current);
+			r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, npages,
+						       readonly, owner,
+						       hmm_range);
+			WRITE_ONCE(p->svms.faulting_task, NULL);
+			if (r) {
+				kfree(hmm_range);
+				hmm_range = NULL;
+				pr_debug("failed %d to get svm range pages\n", r);
+			}
 		}
 
 		if (!r) {
-- 
2.49.0

Re: [PATCH 1/3] drm/amdgpu: svm check hmm range kzalloc return NULL

[Chen, Xiaogang]

On 10/2/2025 12:43 PM, Philip Yang wrote:
> Add hmm_range kzalloc return NULL error check. In case the get_pages
> return failed, free and set hmm_range to NULL, to avoid double free in
> get_pages_done.
>
> Fixes: 29e6f5716115 ("drm/amdgpu: use user provided hmm_range buffer in amdgpu_ttm_tt_get_user_pages")
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 8c3787b00f36..e8a15751c125 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -1736,15 +1736,20 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>   				continue;
>   			}
>   
> -			WRITE_ONCE(p->svms.faulting_task, current);
>   			hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
> -			r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, npages,
> -						       readonly, owner,
> -						       hmm_range);
> -			WRITE_ONCE(p->svms.faulting_task, NULL);
> -			if (r) {
> -				kfree(hmm_range);
> -				pr_debug("failed %d to get svm range pages\n", r);
> +			if (unlikely(!hmm_range)) {
> +				r = -ENOMEM;
> +			} else {
> +				WRITE_ONCE(p->svms.faulting_task, current);
> +				r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, npages,
> +							       readonly, owner,
> +							       hmm_range);
> +				WRITE_ONCE(p->svms.faulting_task, NULL);
> +				if (r) {
> +					kfree(hmm_range);
> +					hmm_range = NULL;

How it avoid double free hmm_range? Currently hmm_range got freed at 
amdgpu_hmm_range_get_pages_done. You free it here, then 
amdgpu_hmm_range_get_pages_done would not free it. if do not free here  
amdgpu_hmm_range_get_pages_done would do.

And besides free hmm_range, we also need to free hmm_range->hmm_pfns 
that is done at amdgpu_hmm_range_get_pages_done.

I think the real problem is hmn_range is allocated and free at different 
places. I do not know why.

Regards

Xiaogang


> +					pr_debug("failed %d to get svm range pages\n", r);
> +				}
>   			}
>   		} else {
>   			r = -EFAULT;

Re: [PATCH 2/3] drm/amdkfd: svm unmap use page aligned address

[Chen, Xiaogang]

On 10/2/2025 12:43 PM, Philip Yang wrote:
> svm_range_unmap_from_gpus uses page aligned start, end address, the end
> address is inclusive.
>
> Fixes: 38c55f6719f7 ("drm/amdkfd: Handle lack of READ permissions in SVM mapping")
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index e8a15751c125..742c28833650 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -1723,8 +1723,8 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>   				svm_range_lock(prange);
>   				if (vma->vm_flags & VM_WRITE)
>   					pr_debug("VM_WRITE without VM_READ is not supported");
> -				s = max(start, prange->start);
> -				e = min(end, prange->last);
> +				s = max(start >> PAGE_SHIFT, prange->start);
> +				e = min((end - 1) >> PAGE_SHIFT, prange->last);

I think the problem is more than that. Here "end" is the last place for 
prange gpu mapping and the handling here is for a vma. Should use end of 
the vma range to get "e".

Regards

Xiaogang

>   				if (e >= s)
>   					r = svm_range_unmap_from_gpus(prange, s, e,
>   						       KFD_SVM_UNMAP_TRIGGER_UNMAP_FROM_CPU);

Re: [PATCH 2/3] drm/amdkfd: svm unmap use page aligned address

[Philip Yang]

On 2025-10-02 18:04, Chen, Xiaogang wrote:
>
> On 10/2/2025 12:43 PM, Philip Yang wrote:
>> svm_range_unmap_from_gpus uses page aligned start, end address, the end
>> address is inclusive.
>>
>> Fixes: 38c55f6719f7 ("drm/amdkfd: Handle lack of READ permissions in 
>> SVM mapping")
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> index e8a15751c125..742c28833650 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> @@ -1723,8 +1723,8 @@ static int svm_range_validate_and_map(struct 
>> mm_struct *mm,
>>                   svm_range_lock(prange);
>>                   if (vma->vm_flags & VM_WRITE)
>>                       pr_debug("VM_WRITE without VM_READ is not 
>> supported");
>> -                s = max(start, prange->start);
>> -                e = min(end, prange->last);
>> +                s = max(start >> PAGE_SHIFT, prange->start);
>> +                e = min((end - 1) >> PAGE_SHIFT, prange->last);
>
> I think the problem is more than that. Here "end" is the last place 
> for prange gpu mapping and the handling here is for a vma. Should use 
> end of the vma range to get "e".

You are right, end should use next - 1 instead, I will refactor the 
patch series, send out v2.

regards,

Philip

>
> Regards
>
> Xiaogang
>
>>                   if (e >= s)
>>                       r = svm_range_unmap_from_gpus(prange, s, e,
>> KFD_SVM_UNMAP_TRIGGER_UNMAP_FROM_CPU);

Re: [PATCH 1/3] drm/amdgpu: svm check hmm range kzalloc return NULL

[Philip Yang]

On 2025-10-02 17:48, Chen, Xiaogang wrote:
>
> On 10/2/2025 12:43 PM, Philip Yang wrote:
>> Add hmm_range kzalloc return NULL error check. In case the get_pages
>> return failed, free and set hmm_range to NULL, to avoid double free in
>> get_pages_done.
>>
>> Fixes: 29e6f5716115 ("drm/amdgpu: use user provided hmm_range buffer 
>> in amdgpu_ttm_tt_get_user_pages")
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> index 8c3787b00f36..e8a15751c125 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> @@ -1736,15 +1736,20 @@ static int svm_range_validate_and_map(struct 
>> mm_struct *mm,
>>                   continue;
>>               }
>>   -            WRITE_ONCE(p->svms.faulting_task, current);
>>               hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
>> -            r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, 
>> npages,
>> -                               readonly, owner,
>> -                               hmm_range);
>> -            WRITE_ONCE(p->svms.faulting_task, NULL);
>> -            if (r) {
>> -                kfree(hmm_range);
>> -                pr_debug("failed %d to get svm range pages\n", r);
>> +            if (unlikely(!hmm_range)) {
>> +                r = -ENOMEM;
>> +            } else {
>> +                WRITE_ONCE(p->svms.faulting_task, current);
>> +                r = amdgpu_hmm_range_get_pages(&prange->notifier, 
>> addr, npages,
>> +                                   readonly, owner,
>> +                                   hmm_range);
>> +                WRITE_ONCE(p->svms.faulting_task, NULL);
>> +                if (r) {
>> +                    kfree(hmm_range);
>> +                    hmm_range = NULL;
>
> How it avoid double free hmm_range? Currently hmm_range got freed at 
> amdgpu_hmm_range_get_pages_done. You free it here, then 
> amdgpu_hmm_range_get_pages_done would not free it. if do not free 
> here  amdgpu_hmm_range_get_pages_done would do.
>
> And besides free hmm_range, we also need to free hmm_range->hmm_pfns 
> that is done at amdgpu_hmm_range_get_pages_done.

if amdgpu_hmm_range_get_pages returns failed, hmm_range->hmm_pfns is 
already freed. If only free hmm_range, but don't set hmm_range to NULL, 
then amdgpu_hmm_range_get_pages_done is called and will double free 
hmm_range again.

>
> I think the real problem is hmn_range is allocated and free at 
> different places. I do not know why.

This is the change in 29e6f5716115, with details in the patch.

Regards,

Philip

>
> Regards
>
> Xiaogang
>
>
>> +                    pr_debug("failed %d to get svm range pages\n", r);
>> +                }
>>               }
>>           } else {
>>               r = -EFAULT;

Re: [PATCH 1/3] drm/amdgpu: svm check hmm range kzalloc return NULL

[Chen, Xiaogang]

On 10/3/2025 10:12 AM, Philip Yang wrote:
>
> On 2025-10-02 17:48, Chen, Xiaogang wrote:
>>
>> On 10/2/2025 12:43 PM, Philip Yang wrote:
>>> Add hmm_range kzalloc return NULL error check. In case the get_pages
>>> return failed, free and set hmm_range to NULL, to avoid double free in
>>> get_pages_done.
>>>
>>> Fixes: 29e6f5716115 ("drm/amdgpu: use user provided hmm_range buffer 
>>> in amdgpu_ttm_tt_get_user_pages")
>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 21 +++++++++++++--------
>>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index 8c3787b00f36..e8a15751c125 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -1736,15 +1736,20 @@ static int svm_range_validate_and_map(struct 
>>> mm_struct *mm,
>>>                   continue;
>>>               }
>>>   -            WRITE_ONCE(p->svms.faulting_task, current);
>>>               hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
>>> -            r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, 
>>> npages,
>>> -                               readonly, owner,
>>> -                               hmm_range);
>>> -            WRITE_ONCE(p->svms.faulting_task, NULL);
>>> -            if (r) {
>>> -                kfree(hmm_range);
>>> -                pr_debug("failed %d to get svm range pages\n", r);
>>> +            if (unlikely(!hmm_range)) {
>>> +                r = -ENOMEM;
>>> +            } else {
>>> +                WRITE_ONCE(p->svms.faulting_task, current);
>>> +                r = amdgpu_hmm_range_get_pages(&prange->notifier, 
>>> addr, npages,
>>> +                                   readonly, owner,
>>> +                                   hmm_range);
>>> +                WRITE_ONCE(p->svms.faulting_task, NULL);
>>> +                if (r) {
>>> +                    kfree(hmm_range);
>>> +                    hmm_range = NULL;
>>
>> How it avoid double free hmm_range? Currently hmm_range got freed at 
>> amdgpu_hmm_range_get_pages_done. You free it here, then 
>> amdgpu_hmm_range_get_pages_done would not free it. if do not free 
>> here  amdgpu_hmm_range_get_pages_done would do.
>>
>> And besides free hmm_range, we also need to free hmm_range->hmm_pfns 
>> that is done at amdgpu_hmm_range_get_pages_done.
>
> if amdgpu_hmm_range_get_pages returns failed, hmm_range->hmm_pfns is 
> already freed. If only free hmm_range, but don't set hmm_range to 
> NULL, then amdgpu_hmm_range_get_pages_done is called and will double 
> free hmm_range again.

Yes, but amdgpu_hmm_range_get_pages_done already has hmm_range free, why 
you free hmm_range at svm?

I think the thing got confusing is hmm_range allocation and free at 
different places, not double free. If hmm_range is allocated by svm, svm 
needs free no matter amdgpu_hmm_range_get_pages is success or not. Even 
have allocation and free at different places the free function should 
appear only in once place.

>
>>
>> I think the real problem is hmn_range is allocated and free at 
>> different places. I do not know why.
>
> This is the change in 29e6f5716115, with details in the patch.
>
This patch wants have hmm_range pointer easier, but I think allocation 
and free hmm_ranges at different components is not a good idea.

Regards

Xiaogang

> Regards,
>
> Philip
>
>>
>> Regards
>>
>> Xiaogang
>>
>>
>>> +                    pr_debug("failed %d to get svm range pages\n", r);
>>> +                }
>>>               }
>>>           } else {
>>>               r = -EFAULT;