Re: [PATCH] drm/nouveau: idle the channel before destroying an engine object via abi16

[sashiko-bot@kernel.org]

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 <xnderv@yandex.com>

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/20261023144219.46478-1-xnderv@yandex.com?part=1