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
