Re: [PATCH v4 2/3] drm/nouveau: don't kill a fence context that is not ready yet

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The newly introduced `ready` flag check can be bypassed because `chan->fence` is published without a release barrier by the backend allocators, allowing a concurrent `nouveau_channel_kill()` to observe pre-initialization garbage.
- [High] A concurrent channel kill event can trigger a Use-After-Free if fence context initialization fails because the kill event is armed prior to full initialization.
--

--- Patch [2]: [PATCH v4 2/3] drm/nouveau: don't kill a fence context that is not ready yet ---
commit 566dba46ab74848a1cc990a88e4d8e18c316f52d
Author: Marek Czernohous <[email protected]>

drm/nouveau: don't kill a fence context that is not ready yet

This commit introduces a ready flag to the fence context to prevent the
kill handler from taking an uninitialized lock. It uses smp_store_release()
and smp_load_acquire() to hand over the context safely once it is fully
built. A channel killed during initialization is recorded and processed
once armed.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> @@ -43,9 +43,17 @@ module_param_named(vram_pushbuf, nouveau_vram_pushbuf, int, 0400);
>  void
>  nouveau_channel_kill(struct nouveau_channel *chan)
>  {
> +	struct nouveau_fence_chan *fctx;
> +
>  	atomic_set(&chan->killed, 1);
> -	if (chan->fence)
> -		nouveau_fence_context_kill(chan->fence, -ENODEV);
> +
> +	/* Pairs with the smp_mb() in nouveau_fence_context_arm(). */
> +	smp_mb();
> +
> +	fctx = READ_ONCE(chan->fence);
> +	/* Pairs with the smp_store_release() there. */
> +	if (fctx && smp_load_acquire(&fctx->ready))

[Severity: High]
This is a pre-existing issue, but can we read pre-initialization garbage
for fctx->ready here?

Because chan->fence is published by backend allocators with a plain store
(e.g., chan->fence = kzalloc_obj(*fctx)), on weakly-ordered architectures,
this READ_ONCE() might observe the pointer before the allocator's zeroing
stores are visible. The smp_load_acquire() on fctx->ready doesn't prevent
reading pre-initialization garbage for the ready flag itself.

If ready is read as non-zero garbage, the kill handler would proceed to
acquire an uninitialized spinlock, potentially causing a kernel panic.

> +		nouveau_fence_context_kill(fctx, -ENODEV);
>  }

[ ... ]

> @@ -494,7 +502,12 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
>  	}
>  
>  	/* initialise synchronisation */
> -	return nouveau_fence(drm)->context_new(chan);
> +	ret = nouveau_fence(drm)->context_new(chan);
> +	if (ret)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but could this error path lead
to a use-after-free if a concurrent channel kill event fires?

Earlier in nouveau_channel_init(), the kill event is armed:

drivers/gpu/drm/nouveau/nouveau_chan.c:nouveau_channel_init() {
    ...
		if (ret == 0)
			ret = nvif_event_allow(&chan->kill);
    ...
}

If context_new() fails (e.g., due to VMA allocation failure), it cleans up
by freeing the fence context. If a hardware kill event fires concurrently,
nouveau_channel_kill() might read chan->fence before the error path clears
it. This would lead to a use-after-free when smp_load_acquire(&fctx->ready)
is evaluated on the freed pointer in the asynchronous kill handler.

> +
> +	nouveau_fence_context_arm(chan);
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2