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 <[email protected]> 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/[email protected]?part=1
