Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [High] accel/qaic: unlocked bo->dbc dereference in qaic_wait_bo_ioctl() It also noticed 1 related pre-existing issue this patch does not introduce: - [Medium] accel/qaic: missing completion signal in send_bo_list_to_device() error path [1] --
commit f3a92ee30bd740f37f373a9b22ee385311742a39 Author: Youssef Samir <[email protected]> accel/qaic: Harden checks in BO execute and wait IOCTLs This commit adds checks in send_bo_list_to_device() and qaic_wait_bo_ioctl() to verify that BOs are sliced and attached to the specified DBC. > diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c > index ee35e13091b30..be3ceb8c51a09 100644 > --- a/drivers/accel/qaic/qaic_data.c > +++ b/drivers/accel/qaic/qaic_data.c > @@ -1256,6 +1256,11 @@ static int send_bo_list_to_device(struct qaic_device > *qdev, struct drm_file *fil > goto unlock_bo; > } > > + if (bo->dbc->id != dbc->id) { > + ret = -EINVAL; > + goto unlock_bo; > + } > + > if (is_partial && pexec[i].resize > bo->base.size) { > ret = -EINVAL; > goto unlock_bo; [Severity: Medium] This problem wasn't introduced by this patch, but can failing on a later BO in the batch leave waiters on earlier BOs hung indefinitely [1]? For each BO processed before the failure, send_bo_list_to_device() calls reinit_completion(&bo->xfer_done) and adds the BO to dbc->xfer_list. When a subsequent BO jumps to unlock_bo and failed_to_send_bo, the rollback loop removes those earlier BOs from dbc->xfer_list without calling complete_all(&bo->xfer_done): drivers/accel/qaic/qaic_data.c:send_bo_list_to_device() { ... failed_to_send_bo: if (likely(obj)) drm_gem_object_put(obj); for (j = 0; j < i; j++) { spin_lock_irqsave(&dbc->xfer_lock, flags); bo = list_last_entry(&dbc->xfer_list, struct qaic_bo, xfer_list); obj = &bo->base; list_del_init(&bo->xfer_list); spin_unlock_irqrestore(&dbc->xfer_lock, flags); dma_sync_sgtable_for_cpu(&qdev->pdev->dev, bo->sgt, bo->dir); drm_gem_object_put(obj); } } Because the rolled-back BOs are no longer on dbc->xfer_list, the interrupt handler will never complete them, so any thread waiting in qaic_wait_bo_ioctl() will block until timeout. > @@ -1769,6 +1774,17 @@ int qaic_wait_bo_ioctl(struct drm_device *dev, void > *data, struct drm_file *file > } > > bo = to_qaic_bo(obj); > + > + if (!bo->sliced) { > + ret = -EINVAL; > + goto put_obj; > + } > + > + if (bo->dbc->id != dbc->id) { > + ret = -EINVAL; > + goto put_obj; > + } > + > timeout = args->timeout ? args->timeout : wait_exec_default_timeout_ms; > timeout = msecs_to_jiffies(timeout); > ret = wait_for_completion_interruptible_timeout(&bo->xfer_done, > timeout); [Severity: High] Can this dereference a NULL bo->dbc when racing with qaic_detach_slice_bo_ioctl()? bo->sliced and bo->dbc are protected by bo->lock, which qaic_wait_bo_ioctl() does not hold. A concurrent detach clears bo->dbc in qaic_unprepare_bo() before clearing bo->sliced in qaic_init_bo(): CPU0 CPU1 qaic_wait_bo_ioctl() qaic_detach_slice_bo_ioctl() mutex_lock(&bo->lock) detach_slice_bo() qaic_unprepare_bo() bo->dbc = NULL if (!bo->sliced) if (bo->dbc->id != dbc->id) <-- NULL pointer dereference qaic_init_bo() bo->sliced = false Acquiring bo->lock around the bo->sliced and bo->dbc->id checks prevents the race. [1] https://sashiko.dev/#/bug/linux-3d10e194-452c-43b5-a7b5-0f068986229f -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
