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.y...@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;