Re: [PATCH v2 3/3] drm/nouveau/fifo: add recovery path for Tesla cache_error/dma_pusher

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Race condition between nvkm_chan_del() and nv04_fifo_recover() leaves a dangling pointer in wedge.chfault, allowing a newly allocated channel to incorrectly inherit a fault streak.
--

--- Patch [3]: [PATCH v2 3/3] drm/nouveau/fifo: add recovery path for Tesla cache_error/dma_pusher ---
commit 673f2147a13507bd6f62792657604b061ac497d7
Author: Marek Czernohous <[email protected]>

drm/nouveau/fifo: add recovery path for Tesla cache_error/dma_pusher

This commit adds a shared recovery helper, nv04_fifo_recover, for Tesla
chipset FIFO fault handling. It implements a two-tier recovery mechanism
that kills channels after a burst of faults and requests a device-wide
wedge event if faults continue.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> index 418a8918bcb8f..79774c6460f4f 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c

[ ... ]

> @@ -275,6 +275,20 @@ nvkm_chan_del(struct nvkm_chan **pchan)
>  	nvkm_gpuobj_del(&chan->ramfc);
>  
>  	if (chan->cgrp) {
> +		struct nvkm_fifo *fifo = chan->cgrp->runl->fifo;
> +
> +		/* Drop this channel's fault streak before the id is reused. */
> +		if (chan->id >= 0) {
> +			typeof(&fifo->wedge.chfault[0]) cf =
> +				&fifo->wedge.chfault[chan->id % NVKM_FIFO_KILL_CHID_MAX];
> +			unsigned long flags;
> +
> +			spin_lock_irqsave(&fifo->wedge.lock, flags);
> +			if (cf->owner == chan)
> +				cf->owner = NULL;
> +			spin_unlock_irqrestore(&fifo->wedge.lock, flags);

[Severity: Medium]
Can a concurrent nv04_fifo_recover() race with this teardown and leave a
dangling pointer in cf->owner?

Because this is cleared before nvkm_chid_put() unlinks the channel, and
before chan->cgrp->lock is acquired, is it possible for the following
sequence to occur:

CPU1 in nvkm_chan_del():
    spin_lock_irqsave(&fifo->wedge.lock, flags);
    if (cf->owner == chan)
        cf->owner = NULL;
    spin_unlock_irqrestore(&fifo->wedge.lock, flags);

CPU2 handling an interrupt in nv04_fifo_recover():
    chan = nvkm_chan_get_chid(...) 
    ...
    spin_lock_irqsave(&w->lock, wflags);
    if (cf->owner != chan || ...) {
        cf->owner = chan;
    }

CPU1 then calls nvkm_chid_put() unlinking the channel, and it is freed.

Would this leave the freed memory address in cf->owner, causing a newly
allocated channel that happens to reuse this address to incorrectly inherit
the previous channel's fault streak?

> +		}
> +
>  		nvkm_chid_put(chan->cgrp->runl->chid, chan->id, &chan->cgrp->lock);
>  		nvkm_cgrp_unref(&chan->cgrp);
>  	}

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