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

[Felix Kuehling <felix.kuehling@amd.com>]

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;