* Re: [PATCH v2] amdkfd: fix doorbell allocation race creating duplicate allocations
2026-10-10 3:02 ` [PATCH v2] " Henry Martin
@ 2026-10-10 3:15 ` sashiko-bot
2026-10-13 17:39 ` Felix Kuehling
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-10-10 3:15 UTC (permalink / raw)
To: Henry Martin; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] amdkfd: Double-Checked Locking vulnerability exposes unpinned doorbell BO to mmap
- [High] drm/amdkfd: TOCTOU race in doorbell allocation exposing unpinned BO
--
commit 8544c203ebbbee60344c5d18d017125a93e18d7c
Author: Henry Martin <bsdhenrymartin@gmail.com>
amdkfd: fix doorbell allocation race creating duplicate allocations
The commit adds a per-process-device mutex doorbell_mutex inside
kfd_alloc_process_doorbells() to fix a check-and-allocate race condition
where concurrent callers could create duplicate doorbell allocations.
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> index fdcf7f2d1b5b4..55acadc986bee 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> @@ -257,12 +257,21 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
> int r;
> struct qcm_process_device *qpd = &pdd->qpd;
>
> + mutex_lock(&pdd->doorbell_mutex);
> +
> + /* Another thread may have won the check-and-allocate race */
> + if (qpd->proc_doorbells) {
> + mutex_unlock(&pdd->doorbell_mutex);
> + return 0;
> + }
> +
> /* Allocate bitmap for dynamic doorbell allocation */
> qpd->doorbell_bitmap = bitmap_zalloc(KFD_MAX_NUM_OF_QUEUES_PER_PROCESS,
[ ... ]
> /* Allocate doorbells for this process */
> r = amdgpu_bo_create_kernel(kfd->adev,
> kfd_doorbell_process_slice(kfd),
> PAGE_SIZE,
> AMDGPU_GEM_DOMAIN_DOORBELL,
> &qpd->proc_doorbells,
> NULL,
> NULL);
> if (r) {
> DRM_ERROR("Failed to allocate process doorbells\n");
> goto err;
> }
> +
> + mutex_unlock(&pdd->doorbell_mutex);
[Severity: Critical]
Does moving the mutex inside this function create a double-checked locking
vulnerability since the outer check is lockless?
When kfd_get_process_doorbells() is called via mmap, it does a lockless check:
if (!pdd->qpd.proc_doorbells) {
if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
/* phys_addr_t 0 is error */
return 0;
}
Concurrently, if another thread is inside kfd_alloc_process_doorbells() and
calls amdgpu_bo_create_kernel() while holding the doorbell_mutex, it assigns
the new BO pointer to &qpd->proc_doorbells before the BO is fully pinned.
Can the thread calling mmap() observe the non-NULL proc_doorbells pointer,
bypass the mutex entirely, and use the unpinned BO? If it calls
amdgpu_bo_gpu_offset() on the unpinned BO, could it calculate a bogus physical
address and map unintended memory to userspace?
> return 0;
>
> err:
> bitmap_free(qpd->doorbell_bitmap);
> qpd->doorbell_bitmap = NULL;
> +unlock:
> + mutex_unlock(&pdd->doorbell_mutex);
> return r;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261010030230.1764682-1-bsdhenrymartin@gmail.com?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] amdkfd: fix doorbell allocation race creating duplicate allocations
2026-10-10 3:02 ` [PATCH v2] " Henry Martin
2026-10-10 3:15 ` sashiko-bot
@ 2026-10-13 17:39 ` Felix Kuehling
2026-10-14 3:02 ` henry martin
2026-10-14 15:32 ` [PATCH v3] drm/amdkfd: " Felix Kuehling
2026-10-14 15:49 ` [PATCH v4] " Felix Kuehling
3 siblings, 1 reply; 13+ messages in thread
From: Felix Kuehling @ 2026-10-13 17:39 UTC (permalink / raw)
To: Henry Martin, Alex Deucher, Christian König, David Airlie,
Simona Vetter, Harry Wentland, Alex Hung
Cc: amd-gfx, dri-devel, linux-kernel, stable
On 2026-10-09 23:02, Henry Martin wrote:
> The check-and-allocate sequence on pdd->qpd.proc_doorbells is
> lockless at every call site of kfd_alloc_process_doorbells():
> kfd_get_process_doorbells() (mmap of /dev/kfd),
> kfd_ioctl_create_queue() and the CRIU restore path each test the
> pointer under different locking (or none), so two racing threads can
> both observe NULL and each allocate a doorbell BO and bitmap,
> orphaning one allocation and handing the two mappings different
> backing pages.
>
> Move the serialization into kfd_alloc_process_doorbells() itself,
> using a new per-process-device mutex so all three call sites are
> covered regardless of their own locking. The device-global
> kfd->doorbell_mutex is not suitable here: it is acquired under
> dqm->lock on the kernel-doorbell path (start_cpsch() -> pm_init() ->
> kq_initialize() -> kfd_get_kernel_doorbell()), so holding it across
> the sleeping allocations above would create a lock-ordering
> inversion with dqm->lock via TTM eviction, and it would also
> serialize doorbell setup of unrelated processes behind a
> device-wide lock for per-process state.
>
> This issue was discovered by Tencent CodeBuddy Security.
>
> Cc: stable@vger.kernel.org
> Fixes: 2105a15a2046 ("drm/amdgpu: use doorbell mgr for kfd process doorbells")
I don't think the Fixes tag is correct, even though that's what Claude
suggested. As far as I can see, the code had the same possible race
condition before that change. I believe the problem goes back all the
way to 16f0013157bf ("drm/amdkfd: Allocate doorbells only when needed").
If you don't mind, I'll update that in the commit description before I
submit your patch. With that, the patch is
Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
Thanks,
Felix
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> ---
>
> v2: Use a per-process-device mutex instead of kfd->doorbell_mutex
> (avoids the dqm->lock ordering inversion and the device-wide
> serialization) and correct the Fixes tag. Pointed out by Felix
> Kuehling.
>
> drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c | 15 ++++++++++++++-
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 +++
> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 1 +
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> index fdcf7f2d1b5b4..55acadc986bee 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> @@ -257,12 +257,21 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
> int r;
> struct qcm_process_device *qpd = &pdd->qpd;
>
> + mutex_lock(&pdd->doorbell_mutex);
> +
> + /* Another thread may have won the check-and-allocate race */
> + if (qpd->proc_doorbells) {
> + mutex_unlock(&pdd->doorbell_mutex);
> + return 0;
> + }
> +
> /* Allocate bitmap for dynamic doorbell allocation */
> qpd->doorbell_bitmap = bitmap_zalloc(KFD_MAX_NUM_OF_QUEUES_PER_PROCESS,
> GFP_KERNEL);
> if (!qpd->doorbell_bitmap) {
> DRM_ERROR("Failed to allocate process doorbell bitmap\n");
> - return -ENOMEM;
> + r = -ENOMEM;
> + goto unlock;
> }
>
> r = init_doorbell_bitmap(&pdd->qpd, kfd);
> @@ -284,11 +293,15 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
> DRM_ERROR("Failed to allocate process doorbells\n");
> goto err;
> }
> +
> + mutex_unlock(&pdd->doorbell_mutex);
> return 0;
>
> err:
> bitmap_free(qpd->doorbell_bitmap);
> qpd->doorbell_bitmap = NULL;
> +unlock:
> + mutex_unlock(&pdd->doorbell_mutex);
> return r;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index d8631847f0eb9..96e59133fee02 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -779,6 +779,9 @@ struct kfd_process_device {
> /* per-process-per device QCM data structure */
> struct qcm_process_device qpd;
>
> + /* Serializes process doorbell allocation for this device */
> + struct mutex doorbell_mutex;
> +
> /*Apertures*/
> uint64_t lds_base;
> uint64_t lds_limit;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 0a7c1900da959..4efe4dae79c13 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -1788,6 +1788,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_node *dev,
> pdd->dev = dev;
> INIT_LIST_HEAD(&pdd->qpd.queues_list);
> INIT_LIST_HEAD(&pdd->qpd.priv_queue_list);
> + mutex_init(&pdd->doorbell_mutex);
> pdd->qpd.dqm = dev->dqm;
> pdd->qpd.pqm = &p->pqm;
> pdd->qpd.evicted = 0;
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] amdkfd: fix doorbell allocation race creating duplicate allocations
2026-10-13 17:39 ` Felix Kuehling
@ 2026-10-14 3:02 ` henry martin
0 siblings, 0 replies; 13+ messages in thread
From: henry martin @ 2026-10-14 3:02 UTC (permalink / raw)
To: Felix Kuehling
Cc: Alex Deucher, Christian König, David Airlie, Simona Vetter,
Harry Wentland, Alex Hung, amd-gfx, dri-devel, linux-kernel,
stable
Hi Felix,
No objection at all - 16f0013157bf ("drm/amdkfd: Allocate doorbells
only when needed") looks right to me. Thanks for the archaeology
and for taking care of the tag before submission.
Regards,
Henry
Felix Kuehling <felix.kuehling@amd.com> 于2026年10月14日周三 01:40写道：
>
> On 2026-10-09 23:02, Henry Martin wrote:
> > The check-and-allocate sequence on pdd->qpd.proc_doorbells is
> > lockless at every call site of kfd_alloc_process_doorbells():
> > kfd_get_process_doorbells() (mmap of /dev/kfd),
> > kfd_ioctl_create_queue() and the CRIU restore path each test the
> > pointer under different locking (or none), so two racing threads can
> > both observe NULL and each allocate a doorbell BO and bitmap,
> > orphaning one allocation and handing the two mappings different
> > backing pages.
> >
> > Move the serialization into kfd_alloc_process_doorbells() itself,
> > using a new per-process-device mutex so all three call sites are
> > covered regardless of their own locking. The device-global
> > kfd->doorbell_mutex is not suitable here: it is acquired under
> > dqm->lock on the kernel-doorbell path (start_cpsch() -> pm_init() ->
> > kq_initialize() -> kfd_get_kernel_doorbell()), so holding it across
> > the sleeping allocations above would create a lock-ordering
> > inversion with dqm->lock via TTM eviction, and it would also
> > serialize doorbell setup of unrelated processes behind a
> > device-wide lock for per-process state.
> >
> > This issue was discovered by Tencent CodeBuddy Security.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 2105a15a2046 ("drm/amdgpu: use doorbell mgr for kfd process doorbells")
>
> I don't think the Fixes tag is correct, even though that's what Claude
> suggested. As far as I can see, the code had the same possible race
> condition before that change. I believe the problem goes back all the
> way to 16f0013157bf ("drm/amdkfd: Allocate doorbells only when needed").
> If you don't mind, I'll update that in the commit description before I
> submit your patch. With that, the patch is
>
> Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
>
> Thanks,
> Felix
>
>
> > Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> > ---
> >
> > v2: Use a per-process-device mutex instead of kfd->doorbell_mutex
> > (avoids the dqm->lock ordering inversion and the device-wide
> > serialization) and correct the Fixes tag. Pointed out by Felix
> > Kuehling.
> >
> > drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c | 15 ++++++++++++++-
> > drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 +++
> > drivers/gpu/drm/amd/amdkfd/kfd_process.c | 1 +
> > 3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> > index fdcf7f2d1b5b4..55acadc986bee 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> > @@ -257,12 +257,21 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
> > int r;
> > struct qcm_process_device *qpd = &pdd->qpd;
> >
> > + mutex_lock(&pdd->doorbell_mutex);
> > +
> > + /* Another thread may have won the check-and-allocate race */
> > + if (qpd->proc_doorbells) {
> > + mutex_unlock(&pdd->doorbell_mutex);
> > + return 0;
> > + }
> > +
> > /* Allocate bitmap for dynamic doorbell allocation */
> > qpd->doorbell_bitmap = bitmap_zalloc(KFD_MAX_NUM_OF_QUEUES_PER_PROCESS,
> > GFP_KERNEL);
> > if (!qpd->doorbell_bitmap) {
> > DRM_ERROR("Failed to allocate process doorbell bitmap\n");
> > - return -ENOMEM;
> > + r = -ENOMEM;
> > + goto unlock;
> > }
> >
> > r = init_doorbell_bitmap(&pdd->qpd, kfd);
> > @@ -284,11 +293,15 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
> > DRM_ERROR("Failed to allocate process doorbells\n");
> > goto err;
> > }
> > +
> > + mutex_unlock(&pdd->doorbell_mutex);
> > return 0;
> >
> > err:
> > bitmap_free(qpd->doorbell_bitmap);
> > qpd->doorbell_bitmap = NULL;
> > +unlock:
> > + mutex_unlock(&pdd->doorbell_mutex);
> > return r;
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > index d8631847f0eb9..96e59133fee02 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > @@ -779,6 +779,9 @@ struct kfd_process_device {
> > /* per-process-per device QCM data structure */
> > struct qcm_process_device qpd;
> >
> > + /* Serializes process doorbell allocation for this device */
> > + struct mutex doorbell_mutex;
> > +
> > /*Apertures*/
> > uint64_t lds_base;
> > uint64_t lds_limit;
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > index 0a7c1900da959..4efe4dae79c13 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > @@ -1788,6 +1788,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_node *dev,
> > pdd->dev = dev;
> > INIT_LIST_HEAD(&pdd->qpd.queues_list);
> > INIT_LIST_HEAD(&pdd->qpd.priv_queue_list);
> > + mutex_init(&pdd->doorbell_mutex);
> > pdd->qpd.dqm = dev->dqm;
> > pdd->qpd.pqm = &p->pqm;
> > pdd->qpd.evicted = 0;
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] drm/amdkfd: fix doorbell allocation race creating duplicate allocations
2026-10-10 3:02 ` [PATCH v2] " Henry Martin
2026-10-10 3:15 ` sashiko-bot
2026-10-13 17:39 ` Felix Kuehling
@ 2026-10-14 15:32 ` Felix Kuehling
2026-10-14 15:44 ` sashiko-bot
2026-10-14 15:49 ` [PATCH v4] " Felix Kuehling
3 siblings, 1 reply; 13+ messages in thread
From: Felix Kuehling @ 2026-10-14 15:32 UTC (permalink / raw)
To: bsdhenrymartin, amd-gfx; +Cc: dri-devel, stable
From: Henry Martin <bsdhenrymartin@gmail.com>
The check-and-allocate sequence on pdd->qpd.proc_doorbells is
lockless at every call site of kfd_alloc_process_doorbells():
kfd_get_process_doorbells() (mmap of /dev/kfd),
kfd_ioctl_create_queue() and the CRIU restore path each test the
pointer under different locking (or none), so two racing threads can
both observe NULL and each allocate a doorbell BO and bitmap,
orphaning one allocation and handing the two mappings different
backing pages.
Move the serialization into kfd_alloc_process_doorbells() itself,
using a new per-process-device mutex so all three call sites are
covered regardless of their own locking. The device-global
kfd->doorbell_mutex is not suitable here: it is acquired under
dqm->lock on the kernel-doorbell path (start_cpsch() -> pm_init() ->
kq_initialize() -> kfd_get_kernel_doorbell()), so holding it across
the sleeping allocations above would create a lock-ordering
inversion with dqm->lock via TTM eviction, and it would also
serialize doorbell setup of unrelated processes behind a
device-wide lock for per-process state.
This issue was discovered by Tencent CodeBuddy Security.
v3 (Felix Kuehling):
- Removed unlocked check in kfd_get_process_doorbell flagged as
problematic by Sashiko Bot. Fall back to the locked check inside
kfd_alloc_process_doorbells
- Added missing mutex_destroy
Cc: stable@vger.kernel.org
Fixes: 16f0013157bf ("drm/amdkfd: Allocate doorbells only when needed")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
Signed-off-by: Felix Kuehling <felix.kuehling@amd.com>
Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c | 23 +++++++++++++++++------
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 +++
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 ++
3 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
index fdcf7f2d1b5b4..6af268a031f22 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
@@ -239,11 +239,9 @@ phys_addr_t kfd_get_process_doorbells(struct kfd_process_device *pdd)
struct amdgpu_device *adev = pdd->dev->adev;
uint32_t first_db_index;
- if (!pdd->qpd.proc_doorbells) {
- if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
- /* phys_addr_t 0 is error */
- return 0;
- }
+ if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
+ /* phys_addr_t 0 is error */
+ return 0;
first_db_index = amdgpu_doorbell_index_on_bar(adev,
pdd->qpd.proc_doorbells,
@@ -257,12 +255,21 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
int r;
struct qcm_process_device *qpd = &pdd->qpd;
+ mutex_lock(&pdd->doorbell_mutex);
+
+ /* Another thread may have won the check-and-allocate race */
+ if (qpd->proc_doorbells) {
+ mutex_unlock(&pdd->doorbell_mutex);
+ return 0;
+ }
+
/* Allocate bitmap for dynamic doorbell allocation */
qpd->doorbell_bitmap = bitmap_zalloc(KFD_MAX_NUM_OF_QUEUES_PER_PROCESS,
GFP_KERNEL);
if (!qpd->doorbell_bitmap) {
DRM_ERROR("Failed to allocate process doorbell bitmap\n");
- return -ENOMEM;
+ r = -ENOMEM;
+ goto unlock;
}
r = init_doorbell_bitmap(&pdd->qpd, kfd);
@@ -284,11 +291,15 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
DRM_ERROR("Failed to allocate process doorbells\n");
goto err;
}
+
+ mutex_unlock(&pdd->doorbell_mutex);
return 0;
err:
bitmap_free(qpd->doorbell_bitmap);
qpd->doorbell_bitmap = NULL;
+unlock:
+ mutex_unlock(&pdd->doorbell_mutex);
return r;
}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 7a365fca68e4f..42ed0d42c0064 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -784,6 +784,9 @@ struct kfd_process_device {
/* per-process-per device QCM data structure */
struct qcm_process_device qpd;
+ /* Serializes process doorbell allocation for this device */
+ struct mutex doorbell_mutex;
+
/*Apertures*/
uint64_t lds_base;
uint64_t lds_limit;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index e742ef206abd6..f092029f23257 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1231,6 +1231,7 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
}
atomic_dec(&pdd->dev->kfd->kfd_processes_count);
+ mutex_destroy(&pdd->doorbell_mutex);
kfree(pdd);
p->pdds[i] = NULL;
@@ -1800,6 +1801,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_node *dev,
pdd->dev = dev;
INIT_LIST_HEAD(&pdd->qpd.queues_list);
INIT_LIST_HEAD(&pdd->qpd.priv_queue_list);
+ mutex_init(&pdd->doorbell_mutex);
pdd->qpd.dqm = dev->dqm;
pdd->qpd.pqm = &p->pqm;
pdd->qpd.evicted = 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3] drm/amdkfd: fix doorbell allocation race creating duplicate allocations
2026-10-14 15:32 ` [PATCH v3] drm/amdkfd: " Felix Kuehling
@ 2026-10-14 15:44 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-10-14 15:44 UTC (permalink / raw)
To: Felix Kuehling; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdkfd: Incomplete fix leaves lockless double-checked locking race
--
commit f4e452c37b6ce531530ab68a693284bf20a378b1
Author: Henry Martin <bsdhenrymartin@gmail.com>
drm/amdkfd: fix doorbell allocation race creating duplicate allocations
The patch introduces a per-process-device mutex to serialize the allocation
of doorbells in kfd_alloc_process_doorbells(). It removes the lockless check
in kfd_get_process_doorbells() so that all synchronization is handled internally
by the allocator.
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> index fdcf7f2d1b5b4..6af268a031f22 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> @@ -239,11 +239,9 @@ phys_addr_t kfd_get_process_doorbells(struct kfd_process_device *pdd)
> struct amdgpu_device *adev = pdd->dev->adev;
> uint32_t first_db_index;
>
> - if (!pdd->qpd.proc_doorbells) {
> - if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
> - /* phys_addr_t 0 is error */
> - return 0;
> - }
> + if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
> + /* phys_addr_t 0 is error */
> + return 0;
[Severity: High]
Are the lockless checks in kfd_ioctl_create_queue() and criu_restore_devices()
intentionally left in drivers/gpu/drm/amd/amdkfd/kfd_chardev.c?
While this lockless check was correctly removed here, identical lockless
checks (if (!pdd->qpd.proc_doorbells)) remain in the other two call sites
mentioned in the commit message.
Concurrently, a thread calling kfd_mmap() will allocate the doorbell structures
under doorbell_mutex. A racing thread in kfd_ioctl_create_queue() will read
proc_doorbells locklessly. Without memory barriers like smp_load_acquire(),
this thread can observe a non-NULL proc_doorbells pointer but fail to see the
fully initialized qpd->doorbell_bitmap, or see the BO before it is fully pinned
by amdgpu_bo_create().
Can this lead to a NULL pointer dereference in operations like
find_first_zero_bit() on doorbell_bitmap, or hardware access with an unpinned
BO?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014153246.538155-1-felix.kuehling@amd.com?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4] drm/amdkfd: fix doorbell allocation race creating duplicate allocations
2026-10-10 3:02 ` [PATCH v2] " Henry Martin
` (2 preceding siblings ...)
2026-10-14 15:32 ` [PATCH v3] drm/amdkfd: " Felix Kuehling
@ 2026-10-14 15:49 ` Felix Kuehling
2026-10-14 16:08 ` sashiko-bot
3 siblings, 1 reply; 13+ messages in thread
From: Felix Kuehling @ 2026-10-14 15:49 UTC (permalink / raw)
To: bsdhenrymartin, amd-gfx; +Cc: dri-devel, stable
From: Henry Martin <bsdhenrymartin@gmail.com>
The check-and-allocate sequence on pdd->qpd.proc_doorbells is
lockless at every call site of kfd_alloc_process_doorbells():
kfd_get_process_doorbells() (mmap of /dev/kfd),
kfd_ioctl_create_queue() and the CRIU restore path each test the
pointer under different locking (or none), so two racing threads can
both observe NULL and each allocate a doorbell BO and bitmap,
orphaning one allocation and handing the two mappings different
backing pages.
Move the serialization into kfd_alloc_process_doorbells() itself,
using a new per-process-device mutex so all three call sites are
covered regardless of their own locking. The device-global
kfd->doorbell_mutex is not suitable here: it is acquired under
dqm->lock on the kernel-doorbell path (start_cpsch() -> pm_init() ->
kq_initialize() -> kfd_get_kernel_doorbell()), so holding it across
the sleeping allocations above would create a lock-ordering
inversion with dqm->lock via TTM eviction, and it would also
serialize doorbell setup of unrelated processes behind a
device-wide lock for per-process state.
This issue was discovered by Tencent CodeBuddy Security.
v3 (Felix Kuehling):
- Removed unlocked check in kfd_get_process_doorbell flagged as
problematic by Sashiko Bot. Fall back to the locked check inside
kfd_alloc_process_doorbells
- Added missing mutex_destroy
v4 (Felix Kuehling):
- Removed unlocked checks around other kfd_alloc_process_doorbells calls
Cc: stable@vger.kernel.org
Fixes: 16f0013157bf ("drm/amdkfd: Allocate doorbells only when needed")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
Signed-off-by: Felix Kuehling <felix.kuehling@amd.com>
Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 18 +++++++-----------
drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c | 23 +++++++++++++++++------
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 +++
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 ++
4 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index b3ebbee76088f..85fc450d79d73 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -427,12 +427,10 @@ static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
}
}
- if (!pdd->qpd.proc_doorbells) {
- err = kfd_alloc_process_doorbells(dev->kfd, pdd);
- if (err) {
- pr_debug("failed to allocate process doorbells\n");
- goto err_bind_process;
- }
+ err = kfd_alloc_process_doorbells(dev->kfd, pdd);
+ if (err) {
+ pr_debug("failed to allocate process doorbells\n");
+ goto err_bind_process;
}
err = kfd_queue_acquire_buffers(pdd, &q_properties);
@@ -2455,11 +2453,9 @@ static int criu_restore_devices(struct kfd_process *p,
goto exit;
}
- if (!pdd->qpd.proc_doorbells) {
- ret = kfd_alloc_process_doorbells(dev->kfd, pdd);
- if (ret)
- goto exit;
- }
+ ret = kfd_alloc_process_doorbells(dev->kfd, pdd);
+ if (ret)
+ goto exit;
}
/*
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
index fdcf7f2d1b5b4..6af268a031f22 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
@@ -239,11 +239,9 @@ phys_addr_t kfd_get_process_doorbells(struct kfd_process_device *pdd)
struct amdgpu_device *adev = pdd->dev->adev;
uint32_t first_db_index;
- if (!pdd->qpd.proc_doorbells) {
- if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
- /* phys_addr_t 0 is error */
- return 0;
- }
+ if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
+ /* phys_addr_t 0 is error */
+ return 0;
first_db_index = amdgpu_doorbell_index_on_bar(adev,
pdd->qpd.proc_doorbells,
@@ -257,12 +255,21 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
int r;
struct qcm_process_device *qpd = &pdd->qpd;
+ mutex_lock(&pdd->doorbell_mutex);
+
+ /* Another thread may have won the check-and-allocate race */
+ if (qpd->proc_doorbells) {
+ mutex_unlock(&pdd->doorbell_mutex);
+ return 0;
+ }
+
/* Allocate bitmap for dynamic doorbell allocation */
qpd->doorbell_bitmap = bitmap_zalloc(KFD_MAX_NUM_OF_QUEUES_PER_PROCESS,
GFP_KERNEL);
if (!qpd->doorbell_bitmap) {
DRM_ERROR("Failed to allocate process doorbell bitmap\n");
- return -ENOMEM;
+ r = -ENOMEM;
+ goto unlock;
}
r = init_doorbell_bitmap(&pdd->qpd, kfd);
@@ -284,11 +291,15 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
DRM_ERROR("Failed to allocate process doorbells\n");
goto err;
}
+
+ mutex_unlock(&pdd->doorbell_mutex);
return 0;
err:
bitmap_free(qpd->doorbell_bitmap);
qpd->doorbell_bitmap = NULL;
+unlock:
+ mutex_unlock(&pdd->doorbell_mutex);
return r;
}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 7a365fca68e4f..42ed0d42c0064 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -784,6 +784,9 @@ struct kfd_process_device {
/* per-process-per device QCM data structure */
struct qcm_process_device qpd;
+ /* Serializes process doorbell allocation for this device */
+ struct mutex doorbell_mutex;
+
/*Apertures*/
uint64_t lds_base;
uint64_t lds_limit;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index e742ef206abd6..f092029f23257 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1231,6 +1231,7 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
}
atomic_dec(&pdd->dev->kfd->kfd_processes_count);
+ mutex_destroy(&pdd->doorbell_mutex);
kfree(pdd);
p->pdds[i] = NULL;
@@ -1800,6 +1801,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_node *dev,
pdd->dev = dev;
INIT_LIST_HEAD(&pdd->qpd.queues_list);
INIT_LIST_HEAD(&pdd->qpd.priv_queue_list);
+ mutex_init(&pdd->doorbell_mutex);
pdd->qpd.dqm = dev->dqm;
pdd->qpd.pqm = &p->pqm;
pdd->qpd.evicted = 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4] drm/amdkfd: fix doorbell allocation race creating duplicate allocations
2026-10-14 15:49 ` [PATCH v4] " Felix Kuehling
@ 2026-10-14 16:08 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-10-14 16:08 UTC (permalink / raw)
To: Felix Kuehling; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/amdkfd: pdd memory leak on kfd_init_apertures() error path
--
commit d555bec2fa5191157e4dad2138f047c49150f26e
Author: Henry Martin <bsdhenrymartin@gmail.com>
drm/amdkfd: fix doorbell allocation race creating duplicate allocations
This patch introduces a per-process-device mutex to serialize process doorbell
allocation. It adds mutex initialization and destruction to the kfd process
lifecycle.
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 226f52626e849..51b0df4984945 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
[ ... ]
> @@ -1802,6 +1803,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_node *dev,
> pdd->dev = dev;
> INIT_LIST_HEAD(&pdd->qpd.queues_list);
> INIT_LIST_HEAD(&pdd->qpd.priv_queue_list);
> + mutex_init(&pdd->doorbell_mutex);
> pdd->qpd.dqm = dev->dqm;
> pdd->qpd.pqm = &p->pqm;
> pdd->qpd.evicted = 0;
[Severity: Medium]
Will this mutex and the pdd struct be leaked if kfd_init_apertures() fails?
Looking at create_process(), if kfd_init_apertures() successfully allocates
one or more pdd structs but then encounters an error and fails, the error
handling path jumps to err_init_apertures:
create_process() {
...
err = kfd_init_apertures(process);
if (err != 0)
goto err_init_apertures;
...
err_init_svm_range_list:
kfd_process_free_outstanding_kfd_bos(process);
kfd_process_destroy_pdds(process);
err_init_apertures:
pqm_uninit(&process->pqm);
...
}
Because err_init_apertures bypasses the call to kfd_process_destroy_pdds(),
does this mean the newly initialized doorbell_mutex and the allocated pdd
structs are leaked?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014154959.540809-1-felix.kuehling@amd.com?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread