Re: [PATCH v3] drm/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] drm/amdkfd: Incomplete fix leaves lockless double-checked locking race
--

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

drm/amdkfd: fix doorbell allocation race creating duplicate allocations

The patch introduces a per-process-device mutex to serialize the allocation
of doorbells in kfd_alloc_process_doorbells(). It removes the lockless check
in kfd_get_process_doorbells() so that all synchronization is handled internally
by the allocator.

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> index fdcf7f2d1b5b4..6af268a031f22 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> @@ -239,11 +239,9 @@ phys_addr_t kfd_get_process_doorbells(struct kfd_process_device *pdd)
>  	struct amdgpu_device *adev = pdd->dev->adev;
>  	uint32_t first_db_index;
>  
> -	if (!pdd->qpd.proc_doorbells) {
> -		if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
> -			/* phys_addr_t 0 is error */
> -			return 0;
> -	}
> +	if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
> +		/* phys_addr_t 0 is error */
> +		return 0;

[Severity: High]
Are the lockless checks in kfd_ioctl_create_queue() and criu_restore_devices()
intentionally left in drivers/gpu/drm/amd/amdkfd/kfd_chardev.c?

While this lockless check was correctly removed here, identical lockless
checks (if (!pdd->qpd.proc_doorbells)) remain in the other two call sites
mentioned in the commit message.

Concurrently, a thread calling kfd_mmap() will allocate the doorbell structures
under doorbell_mutex. A racing thread in kfd_ioctl_create_queue() will read
proc_doorbells locklessly. Without memory barriers like smp_load_acquire(),
this thread can observe a non-NULL proc_doorbells pointer but fail to see the
fully initialized qpd->doorbell_bitmap, or see the BO before it is fully pinned
by amdgpu_bo_create().

Can this lead to a NULL pointer dereference in operations like
find_first_zero_bit() on doorbell_bitmap, or hardware access with an unpinned
BO?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014153246.538155-1-felix.kuehling@amd.com?part=1