Re: [PATCH v4 3/3] drm/nouveau: subscribe to channel-kill events on NV50 and newer

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

Pre-existing issues:
- [High] `chan->fence` is published without memory barriers before its fields and spinlock are fully initialized, exposing an uninitialized spinlock to the concurrent channel kill event handler.
--

--- Patch [3]: [PATCH v4 3/3] drm/nouveau: subscribe to channel-kill events on NV50 and newer ---
commit 526eef3958eedb0aae06aba1612c73d287cde11e
Author: Marek Czernohous <[email protected]>

drm/nouveau: subscribe to channel-kill events on NV50 and newer

This commit lowers the class gate for subscribing to channel-killed events
from FERMI_CHANNEL_GPFIFO to NV50_CHANNEL_GPFIFO. This enables NV50 and Tesla
to correctly receive channel error notifications and signal pending fences.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
> index 605ce74c0d155..54e2202cb852f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> @@ -378,7 +378,7 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
>  	if (ret)
>  		return ret;
>  
> -	if (chan->user.oclass >= FERMI_CHANNEL_GPFIFO) {
> +	if (chan->user.oclass >= NV50_CHANNEL_GPFIFO) {
>  		DEFINE_RAW_FLEX(struct nvif_event_v0, args, data,
>  				sizeof(struct nvif_chan_event_v0));
>  		struct nvif_chan_event_v0 *host =

[Severity: High]
This is a pre-existing issue, but this change exposes an existing race condition
to NV50.

By allowing the channel kill event in this block, the event can fire before the
fence context is fully initialized later in nouveau_channel_init():

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

In nv84_fence_context_new() (and similarly in nv50_fence_context_new()), the
fence pointer is published without a memory barrier:

	fctx = chan->fence = kzalloc_obj(*fctx);

If a channel fault occurs and the kill event handler runs concurrently:

drivers/gpu/drm/nouveau/nouveau_chan.c:nouveau_channel_kill() {
	...
	if (fctx && smp_load_acquire(&fctx->ready))
		nouveau_fence_context_kill(fctx, -ENODEV);
}

Because chan->fence was published without a store release, can
smp_load_acquire() observe non-NULL for fctx but read stale, uninitialized
memory for fctx->ready?

If the stale memory evaluates to true, it would proceed to call
nouveau_fence_context_kill(), which acquires an uninitialized spinlock:

drivers/gpu/drm/nouveau/nouveau_fence.c:nouveau_fence_context_kill() {
	...
	spin_lock_irqsave(&fctx->lock, flags);
}

Could this lead to a kernel crash or deadlock?

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