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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] amdkfd: broken double-checked locking exposes unpinned doorbell BO
--

commit b7981a37c1ce3dead747819635554510915a169b
Author: Henry Martin <bsdhenrymartin@gmail.com>

amdkfd: fix doorbell allocation race creating duplicate allocations

This commit adds a double-checked locking mechanism to
kfd_alloc_process_doorbells to prevent racing threads from creating
duplicate doorbell allocations. It checks the proc_doorbells pointer
under the doorbell_mutex to synchronize allocation across lockless
call sites.

> 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);

[ ... ]

> @@ -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;

[Severity: High]
Does this code expose an unpinned buffer object to lockless readers?

Since amdgpu_bo_create_kernel() writes the newly allocated buffer object
directly to the provided pointer (&qpd->proc_doorbells) before completing
the reserve and pin phases, this pointer becomes globally visible to any
thread performing the lockless check in kfd_get_process_doorbells():

    if (!pdd->qpd.proc_doorbells) {
        if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
            /* phys_addr_t 0 is error */
            return 0;
    }

Could a concurrent thread bypass the mutex, observe the partially
initialized buffer object, and use it to access invalid GPU memory offsets
before pinning is complete?  Could this also trigger a use-after-free if
the allocation subsequently fails to pin and is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009101842.3294707-1-bsdhenrymartin@gmail.com?part=1