From: "Christian König" <christian.koenig@amd.com>
To: Vamsi Krishna Brahmajosyula
<vamsi-krishna.brahmajosyula@broadcom.com>,
stable@vger.kernel.org, gregkh@linuxfoundation.org
Cc: Philip.Yang@amd.com, Felix.Kuehling@amd.com,
alexander.deucher@amd.com, Xinhui.Pan@amd.com, airlied@gmail.com,
daniel@ffwll.ch, amd-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
ajay.kaher@broadcom.com, alexey.makhalov@broadcom.com,
vasavi.sirnapalli@broadcom.com, Sasha Levin <sashal@kernel.org>
Subject: Re: [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer
Date: Wed, 13 Nov 2024 13:13:27 +0100 [thread overview]
Message-ID: <7615f2ae-2563-4939-934d-0b02e3f2d10e@amd.com> (raw)
In-Reply-To: <20241113121030.2405520-1-vamsi-krishna.brahmajosyula@broadcom.com>
Am 13.11.24 um 13:10 schrieb Vamsi Krishna Brahmajosyula:
> From: Philip Yang <Philip.Yang@amd.com>
>
> [ 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@amd.com>
> Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
> Acked-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> Signed-off-by: Vamsi Krishna Brahmajosyula <vamsi-krishna.brahmajosyula@broadcom.com>
> ---
> 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.
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);
next prev parent reply other threads:[~2024-11-13 12:13 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-13 12:10 Vamsi Krishna Brahmajosyula
2024-11-13 12:13 ` Christian König [this message]
2024-11-28 0:19 ` Felix Kuehling
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7615f2ae-2563-4939-934d-0b02e3f2d10e@amd.com \
--to=christian.koenig@amd.com \
--cc=Felix.Kuehling@amd.com \
--cc=Philip.Yang@amd.com \
--cc=Xinhui.Pan@amd.com \
--cc=airlied@gmail.com \
--cc=ajay.kaher@broadcom.com \
--cc=alexander.deucher@amd.com \
--cc=alexey.makhalov@broadcom.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sashal@kernel.org \
--cc=stable@vger.kernel.org \
--cc=vamsi-krishna.brahmajosyula@broadcom.com \
--cc=vasavi.sirnapalli@broadcom.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®