* [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer
@ 2024-11-13 12:10 Vamsi Krishna Brahmajosyula
2024-11-13 12:13 ` Christian König
2024-11-28 12:56 ` Sasha Levin
0 siblings, 2 replies; 4+ messages in thread
From: Vamsi Krishna Brahmajosyula @ 2024-11-13 12:10 UTC (permalink / raw)
To: stable, gregkh
Cc: Philip.Yang, Felix.Kuehling, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, amd-gfx, dri-devel, linux-kernel,
ajay.kaher, alexey.makhalov, vasavi.sirnapalli, Felix Kuehling,
Sasha Levin, 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);
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);
--
2.39.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer
2024-11-13 12:10 [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer Vamsi Krishna Brahmajosyula
@ 2024-11-13 12:13 ` Christian König
2024-11-28 0:19 ` Felix Kuehling
2024-11-28 12:56 ` Sasha Levin
1 sibling, 1 reply; 4+ messages in thread
From: Christian König @ 2024-11-13 12:13 UTC (permalink / raw)
To: Vamsi Krishna Brahmajosyula, stable, gregkh
Cc: Philip.Yang, Felix.Kuehling, alexander.deucher, Xinhui.Pan,
airlied, daniel, amd-gfx, dri-devel, linux-kernel, ajay.kaher,
alexey.makhalov, vasavi.sirnapalli, Sasha Levin
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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer
2024-11-13 12:13 ` Christian König
@ 2024-11-28 0:19 ` Felix Kuehling
0 siblings, 0 replies; 4+ messages in thread
From: Felix Kuehling @ 2024-11-28 0:19 UTC (permalink / raw)
To: Christian König, Vamsi Krishna Brahmajosyula, stable, gregkh
Cc: Philip.Yang, alexander.deucher, Xinhui.Pan, airlied, daniel,
amd-gfx, dri-devel, linux-kernel, ajay.kaher, alexey.makhalov,
vasavi.sirnapalli, Sasha Levin
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@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.
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);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer
2024-11-13 12:10 [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer Vamsi Krishna Brahmajosyula
2024-11-13 12:13 ` Christian König
@ 2024-11-28 12:56 ` Sasha Levin
1 sibling, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2024-11-28 12:56 UTC (permalink / raw)
To: stable; +Cc: Vamsi Krishna Brahmajosyula, Sasha Levin
[ Sasha's backport helper bot ]
Hi,
The upstream commit SHA1 provided is correct: c86ad39140bbcb9dc75a10046c2221f657e8083b
WARNING: Author mismatch between patch and upstream commit:
Backport author: Vamsi Krishna Brahmajosyula <vamsi-krishna.brahmajosyula@broadcom.com>
Commit author: Philip Yang <Philip.Yang@amd.com>
Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.11.y | Present (different SHA1: 6c9289806591)
6.6.y | Present (different SHA1: 30ceb873cc2e)
6.1.y | Present (different SHA1: e7831613cbbc)
Note: The patch differs from the upstream commit:
---
--- - 2024-11-27 18:53:41.547836112 -0500
+++ /tmp/tmp.rI5o4mVBXo 2024-11-27 18:53:41.540011125 -0500
@@ -1,3 +1,5 @@
+[ 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.
@@ -6,6 +8,8 @@
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 +-
@@ -18,10 +22,10 @@
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 03205e3c37463..c272461d70a9a 100644
+index 5d9a34601a1a..c31e5f9d63da 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
-@@ -364,15 +364,15 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t size,
+@@ -344,15 +344,15 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t size,
return r;
}
@@ -45,10 +49,10 @@
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 66b1c72c81e59..6e591280774b9 100644
+index 4b694886715c..c7672a1d1560 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
-@@ -235,7 +235,7 @@ int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
+@@ -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);
@@ -58,45 +62,45 @@
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 1d9b21628be7e..823f245dc7d08 100644
+index e3cd66c4d95d..f83574107eb8 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
-@@ -423,7 +423,7 @@ static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
+@@ -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:
- err_pdd:
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
-index f4d20adaa0689..6619028dd58ba 100644
+index 27820f0a282d..e2c055abfea9 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
-@@ -907,7 +907,7 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
+@@ -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:
- dev_err(kfd_device,
- "device %x:%x NOT added due to errors\n",
-@@ -925,7 +925,7 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
+ 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);
}
-
- kfree(kfd);
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 4f48507418d2f..420444eb8e982 100644
+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
-@@ -2621,7 +2621,7 @@ static void deallocate_hiq_sdma_mqd(struct kfd_node *dev,
+@@ -2392,7 +2392,7 @@ static void deallocate_hiq_sdma_mqd(struct kfd_dev *dev,
{
WARN(!mqd, "No hiq sdma mqd trunk to free");
@@ -106,10 +110,10 @@
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 50a81da43ce19..d9ae854b69084 100644
+index 623ccd227b7d..c733d6888c30 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
-@@ -225,7 +225,7 @@ void kfd_free_mqd_cp(struct mqd_manager *mm, void *mqd,
+@@ -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) {
@@ -119,12 +123,12 @@
} 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 17e42161b0151..9e29b92eb523d 100644
+index 5bca6abd55ae..9582c9449fff 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
-@@ -1048,7 +1048,7 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
+@@ -1052,7 +1052,7 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
- if (pdd->dev->kfd->shared_resources.enable_mes)
+ if (pdd->dev->shared_resources.enable_mes)
amdgpu_amdkfd_free_gtt_mem(pdd->dev->adev,
- pdd->proc_ctx_bo);
+ &pdd->proc_ctx_bo);
@@ -132,18 +136,21 @@
* 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 21f5a1fb3bf88..36f0460cbffe6 100644
+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
-@@ -204,9 +204,9 @@ static void pqm_clean_queue_resource(struct process_queue_manager *pqm,
- }
-
- if (dev->kfd->shared_resources.enable_mes) {
-- amdgpu_amdkfd_free_gtt_mem(dev->adev, pqn->q->gang_ctx_bo);
-+ amdgpu_amdkfd_free_gtt_mem(dev->adev, &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);
- }
- }
+@@ -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);
+--
+2.39.4
+
---
Results of testing on various branches:
| Branch | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.1.y | Failed | N/A |
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-11-28 13:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-13 12:10 [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer Vamsi Krishna Brahmajosyula
2024-11-13 12:13 ` Christian König
2024-11-28 0:19 ` Felix Kuehling
2024-11-28 12:56 ` Sasha Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox