Hi Felix,
You're right on both counts, thanks for the careful look.
v2 drops kfd->doorbell_mutex entirely and adds a per-process-device
mutex in struct kfd_process_device (initialized in
kfd_create_process_device_data()). The device-global mutex is no
longer held across the sleeping allocations, so the existing
dqm->lock -> doorbell_mutex order on the start_cpsch() -> pm_init()
-> kq_initialize() path cannot invert, and unrelated processes no
longer serialize against each other for per-process doorbell setup.
The Fixes tag is corrected to 2105a15a2046 ("drm/amdgpu: use
doorbell mgr for kfd process doorbells").
Regards,
Henry
Kuehling, Felix <[email protected]> 于2026年10月9日周五 23:33写道：
>
> On 2026-10-09 11:00, Kuehling, Felix wrote:
> > On 2026-10-09 06:18, 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:
> >> take kfd->doorbell_mutex and re-check under it, so all three call
> >> sites are covered regardless of their own locking. The existing
> >> doorbell_mutex users (kfd_get_kernel_doorbell() /
> >> kfd_release_kernel_doorbell()) only guard the device-global
> >> kfd->doorbell_bitmap in short non-sleeping sections and never call
> >> into this path, so there is no nesting.
> >>
> >> This vulnerability was discovered by Tencent CodeBuddy Security.
> >>
> >> Fixes: 428542d91772 ("drm/amd/display: Setup for
> >> mmhubbub3_warmup_mcif with big buffer")
> >> Signed-off-by: Henry Martin <[email protected]>
> >
> > Reviewed-by: Felix Kuehling <[email protected]>
> >
> > I'm applying the patch to amd-staging-drm-next.
>
> An automatic Claude review pointed out two problems with the patch. I
> agree with both:
>
> The Fixes: tag points at an unrelated drm/amd/display commit
> (428542d91772). It must be corrected — 2105a15a2046 ("drm/amdgpu: use
> doorbell mgr for kfd process doorbells") is the commit that introduced
> the allocation being fixed.
>
> The patch holds the device-global kfd->doorbell_mutex across
> bitmap_zalloc(GFP_KERNEL) and amdgpu_bo_create_kernel(), while that same
> mutex is already acquired under dqm_lock via start_cpsch -> pm_init ->
> kq_initialize -> kfd_get_kernel_doorbell(). That is a plausible ABBA
> inversion once reclaim/TTM eviction runs the KFD eviction fence back
> into dqm_lock. Please either post lockdep (CONFIG_PROVE_LOCKING) results
> for a boot + queue-create cycle, or switch to a per-process-device lock,
> which also avoids serialising unrelated processes behind a device-wide
> mutex for per-process state.
>
> Regards,
> Felix
>
>
> >
> > Thanks,
> > Felix
> >
> >
> >> ---
> >> drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c | 15 ++++++++++++++-
> >> 1 file changed, 14 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..07f881573837e 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(&kfd->doorbell_mutex);
> >> +
> >> + /* Another thread may have won the check-and-allocate race */
> >> + if (qpd->proc_doorbells) {
> >> + mutex_unlock(&kfd->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(&kfd->doorbell_mutex);
> >> return 0;
> >>
> >> err:
> >> bitmap_free(qpd->doorbell_bitmap);
> >> qpd->doorbell_bitmap = NULL;
> >> +unlock:
> >> + mutex_unlock(&kfd->doorbell_mutex);
> >> return r;
> >> }
> >>
> >> --
> >> 2.43.7
