Re: [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer

[Felix Kuehling <felix.kuehling@xxxxxxx>]

On 2024-11-13 07:13, Christian König wrote:
> Am 13.11.24 um 13:10 schrieb Vamsi Krishna Brahmajosyula:
> > From: Philip Yang <Philip.Yang@xxxxxxx>
> >
> > [ Upstream commit c86ad39140bbcb9dc75a10046c2221f657e8083b ]
> >
> > Pass pointer reference to amdgpu_bo_unref to clear the correct pointer,
> > otherwise amdgpu_bo_unref clear the local variable, the original pointer
> > not set to NULL, this could cause use-after-free bug.
> >
> > Signed-off-by: Philip Yang <Philip.Yang@xxxxxxx>
> > Reviewed-by: Felix Kuehling <felix.kuehling@xxxxxxx>
> > Acked-by: Christian König <christian.koenig@xxxxxxx>
> > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> > Signed-off-by: Vamsi Krishna Brahmajosyula 
> > <vamsi-krishna.brahmajosyula@xxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c         | 14 +++++++-------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h         |  2 +-
> >   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c           |  2 +-
> >   drivers/gpu/drm/amd/amdkfd/kfd_device.c            |  4 ++--
> >   .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c  |  2 +-
> >   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c       |  2 +-
> >   drivers/gpu/drm/amd/amdkfd/kfd_process.c           |  2 +-
> >   .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c |  4 ++--
> >   8 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > index 5d9a34601a1a..c31e5f9d63da 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > @@ -344,15 +344,15 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct 
> > amdgpu_device *adev, size_t size,
> >       return r;
> >   }
> >   -void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void 
> > *mem_obj)
> > +void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void 
> > **mem_obj)
> >   {
> > -    struct amdgpu_bo *bo = (struct amdgpu_bo *) mem_obj;
> > +    struct amdgpu_bo **bo = (struct amdgpu_bo **) mem_obj;
> >   -    amdgpu_bo_reserve(bo, true);
> > -    amdgpu_bo_kunmap(bo);
> > -    amdgpu_bo_unpin(bo);
> > -    amdgpu_bo_unreserve(bo);
> > -    amdgpu_bo_unref(&(bo));
> > +    amdgpu_bo_reserve(*bo, true);
> > +    amdgpu_bo_kunmap(*bo);
> > +    amdgpu_bo_unpin(*bo);
> > +    amdgpu_bo_unreserve(*bo);
> > +    amdgpu_bo_unref(bo);
> >   }
> >     int amdgpu_amdkfd_alloc_gws(struct amdgpu_device *adev, size_t size,
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > index 4b694886715c..c7672a1d1560 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > @@ -210,7 +210,7 @@ int amdgpu_amdkfd_evict_userptr(struct kgd_mem 
> > *mem, struct mm_struct *mm)
> >   int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t 
> > size,
> >                   void **mem_obj, uint64_t *gpu_addr,
> >                   void **cpu_ptr, bool mqd_gfx9);
> > -void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void 
> > *mem_obj);
> > +void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void 
> > **mem_obj);
>
> Why is that a pointer to a void* in the first place? It looks like all 
> callers should work with an amdgpu_bo object as well.

Historical reasons. When KFD was a separate module, mem_obj was opaque 
to KFD, so the call from KFD to KGD used a void*.

Regards,
  Felix


> Regards,
> Christian.
>
> >   int amdgpu_amdkfd_alloc_gws(struct amdgpu_device *adev, size_t size,
> >                   void **mem_obj);
> >   void amdgpu_amdkfd_free_gws(struct amdgpu_device *adev, void 
> > *mem_obj);
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > index e3cd66c4d95d..f83574107eb8 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > @@ -408,7 +408,7 @@ static int kfd_ioctl_create_queue(struct file 
> > *filep, struct kfd_process *p,
> >     err_create_queue:
> >       if (wptr_bo)
> > -        amdgpu_amdkfd_free_gtt_mem(dev->adev, wptr_bo);
> > +        amdgpu_amdkfd_free_gtt_mem(dev->adev, (void **)&wptr_bo);
> >   err_wptr_map_gart:
> >   err_alloc_doorbells:
> >   err_bind_process:
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > index 27820f0a282d..e2c055abfea9 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > @@ -673,7 +673,7 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
> >   kfd_doorbell_error:
> >       kfd_gtt_sa_fini(kfd);
> >   kfd_gtt_sa_init_error:
> > -    amdgpu_amdkfd_free_gtt_mem(kfd->adev, kfd->gtt_mem);
> > +    amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
> >   alloc_gtt_mem_failure:
> >       if (kfd->gws)
> >           amdgpu_amdkfd_free_gws(kfd->adev, kfd->gws);
> > @@ -693,7 +693,7 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
> >           kfd_doorbell_fini(kfd);
> >           ida_destroy(&kfd->doorbell_ida);
> >           kfd_gtt_sa_fini(kfd);
> > -        amdgpu_amdkfd_free_gtt_mem(kfd->adev, kfd->gtt_mem);
> > +        amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
> >           if (kfd->gws)
> >               amdgpu_amdkfd_free_gws(kfd->adev, kfd->gws);
> >       }
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > index 1b7b29426480..3ab0a796af06 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > @@ -2392,7 +2392,7 @@ static void deallocate_hiq_sdma_mqd(struct 
> > kfd_dev *dev,
> >   {
> >       WARN(!mqd, "No hiq sdma mqd trunk to free");
> >   -    amdgpu_amdkfd_free_gtt_mem(dev->adev, mqd->gtt_mem);
> > +    amdgpu_amdkfd_free_gtt_mem(dev->adev, &mqd->gtt_mem);
> >   }
> >     void device_queue_manager_uninit(struct device_queue_manager *dqm)
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
> > index 623ccd227b7d..c733d6888c30 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
> > @@ -204,7 +204,7 @@ void kfd_free_mqd_cp(struct mqd_manager *mm, void 
> > *mqd,
> >             struct kfd_mem_obj *mqd_mem_obj)
> >   {
> >       if (mqd_mem_obj->gtt_mem) {
> > -        amdgpu_amdkfd_free_gtt_mem(mm->dev->adev, 
> > mqd_mem_obj->gtt_mem);
> > +        amdgpu_amdkfd_free_gtt_mem(mm->dev->adev, 
> > &mqd_mem_obj->gtt_mem);
> >           kfree(mqd_mem_obj);
> >       } else {
> >           kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > index 5bca6abd55ae..9582c9449fff 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > @@ -1052,7 +1052,7 @@ static void kfd_process_destroy_pdds(struct 
> > kfd_process *p)
> >             if (pdd->dev->shared_resources.enable_mes)
> >               amdgpu_amdkfd_free_gtt_mem(pdd->dev->adev,
> > -                           pdd->proc_ctx_bo);
> > +                           &pdd->proc_ctx_bo);
> >           /*
> >            * before destroying pdd, make sure to report availability
> >            * for auto suspend
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> > index 99aa8a8399d6..1918a3c06ac8 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> > @@ -441,9 +441,9 @@ int pqm_destroy_queue(struct 
> > process_queue_manager *pqm, unsigned int qid)
> >             if (dev->shared_resources.enable_mes) {
> >               amdgpu_amdkfd_free_gtt_mem(dev->adev,
> > -                           pqn->q->gang_ctx_bo);
> > +                           &pqn->q->gang_ctx_bo);
> >               if (pqn->q->wptr_bo)
> > -                amdgpu_amdkfd_free_gtt_mem(dev->adev, pqn->q->wptr_bo);
> > +                amdgpu_amdkfd_free_gtt_mem(dev->adev, (void 
> > **)&pqn->q->wptr_bo);
> >             }
> >           uninit_queue(pqn->q);