Re: [PATCH] amdkfd: fix doorbell allocation race creating duplicate allocations

[henry martin]

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 <felix.kuehling@xxxxxxx> 于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 <bsdhenrymartin@xxxxxxxxx>
> >
> > Reviewed-by: Felix Kuehling <felix.kuehling@xxxxxxx>
> >
> > 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