Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/nouveau: race between nouveau_abi16_ioctl_del() and chan->sched jobs --
commit 4bc27237b92dfdc3db9914229dd5c55e2c5a4d5a Author: Alex <[email protected]> drm/nouveau: idle the channel before destroying an engine object via abi16 This commit records the owning channel on engine objects created via nouveau_abi16_ioctl_new() and idles that channel in nouveau_abi16_ioctl_del() before destroying the engine object. When a channel is destroyed first in nouveau_abi16_chan_fini(), any channel references held by engine objects are cleared. > diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c > b/drivers/gpu/drm/nouveau/nouveau_abi16.c > index edf867c1fdcb0..f130a127d8bb2 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_abi16.c > +++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c [ ... ] > @@ -788,8 +796,16 @@ nouveau_abi16_ioctl_del(struct nouveau_abi16 *abi16, > struct nvif_ioctl_v0 *ioctl > > obj = nouveau_abi16_obj_find(abi16, ioctl->object); > if (obj) { > - if (obj->type == ENGOBJ) > + if (obj->type == ENGOBJ) { > + /* Synchronize with the user channel before destroying > the > + * engine object, as the legacy GPUOBJ_FREE path does. > The > + * channel may still have work queued that needs the > engine > + * context which is about to be unbound. > + */ > + if (obj->chan && obj->chan->chan) > + nouveau_channel_idle(obj->chan->chan); > nvif_object_dtor(&obj->engobj); > + } [Severity: High] Can calling nouveau_channel_idle(obj->chan->chan) here race with jobs running on obj->chan->sched when VM_BIND is enabled? When a client initializes VM_BIND via DRM_IOCTL_NOUVEAU_VM_INIT, nouveau_abi16_ioctl_channel_alloc() creates a per-channel scheduler in chan->sched, and DRM_IOCTL_NOUVEAU_EXEC submits jobs to chan->sched->entity. Those jobs execute nouveau_exec_job_run() asynchronously on drm->sched_wq without holding cli->mutex: drivers/gpu/drm/nouveau/nouveau_exec.c:nouveau_exec_job_run() { ... for (i = 0; i < exec_job->push.count; i++) { struct drm_nouveau_exec_push *p = &exec_job->push.s[i]; bool no_prefetch = p->flags & DRM_NOUVEAU_EXEC_PUSH_NO_PREFETCH; nvif_chan_gpfifo_push(&chan->chan, p->va, p->va_len, no_prefetch); } nvif_chan_gpfifo_post(&chan->chan); ret = nouveau_fence_emit(fence); ... } Meanwhile, nouveau_channel_idle() emits a fence directly onto the channel via nouveau_fence_new() -> nouveau_fence_emit(): drivers/gpu/drm/nouveau/nouveau_fence.c:nouveau_fence_emit() { ... if (priv->uevent) dma_fence_init(&fence->base, &nouveau_fence_ops_uevent, &fctx->lock, fctx->context, ++fctx->sequence); else dma_fence_init(&fence->base, &nouveau_fence_ops_legacy, &fctx->lock, fctx->context, ++fctx->sequence); kref_get(&fctx->fence_ref); ret = fctx->emit(fence); ... } Unlike nouveau_abi16_chan_fini(), which stops chan->sched->entity before idling the channel, calling nouveau_channel_idle() in nouveau_abi16_ioctl_del() without synchronizing with obj->chan->sched allows nouveau_fence_emit() and fctx->emit() to run concurrently with nouveau_exec_job_run(), modifying fctx->sequence, chan->chan.push, and chan->chan.gpfifo without synchronization. In addition, if DRM_NOUVEAU_EXEC jobs are still queued in obj->chan->sched->entity (for example, waiting on in_sync dependencies), they will not have been pushed to the hardware ring yet when nouveau_channel_idle() waits on its fence, so nvif_object_dtor(&obj->engobj) can still unbind the engine context before those queued jobs run on the GPU. Should nouveau_abi16_ioctl_del() drain or synchronize with obj->chan->sched before calling nouveau_channel_idle(obj->chan->chan)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
