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 <[email protected]> 于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: [email protected]
> > 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 <[email protected]>
>
> Thanks,
> Felix
>
>
> > Signed-off-by: Henry Martin <[email protected]>
> > ---
> >
> > 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;
