nouveau: fix client work fence deletion race
Subsystems (3)
Dave Airlie
maintainerPATCH 1/1 nouveau: fix client work fence deletion race
From: Dave Airlie <[email protected]>
This seems to have existed for ever but is now more apparant after 9bff18d13473a9fdf81d5158248472a9d8ecf2bd (drm/ttm: use per BO cleanup workers)
My analysis:
two threads are running,
one in the irq signalling the fence, in dma_fence_signal_timestamp_locked, it has done the DMA_FENCE_FLAG_SIGNALLED_BIT setting, but hasn't yet reached the callbacks.
second thread in nouveau_cli_work_ready, where it sees the fence is signalled, so then puts the fence, cleanups the object and frees the work item, which contains the callback.
thread one goes again and tries to call the callback and causes the use-after-free.
Proposed fix:
lock the fence signalled check in nouveau_cli_work_ready, so either the callbacks are done or the memory is freed.
--- drivers/gpu/drm/nouveau/nouveau_drm.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/gpu/drm/nouveau/nouveau_drm.c +10 -4
@@ -137,10 +137,16 @@ nouveau_name(struct drm_device *dev)static inline bool nouveau_cli_work_ready (struct dma_fence *fence) { -if (!dma_fence_is_signaled (fence)) -return false; -dma_fence_put (fence); -return true; +unsigned long flags; +bool ret = true; +spin_lock_irqsave (fence->lock, flags); +if (!dma_fence_is_signaled_locked (fence)) + ret = false; +spin_unlock_irqrestore (fence->lock, flags); + +if (ret == true) +dma_fence_put (fence); +return ret; } static void
Signature · 1 line
2.40.1
Quoted 14 lines · Dave Airlie
From: Dave Airlie <[email protected]>
This seems to have existed for ever but is now more apparant after
9bff18d13473a9fdf81d5158248472a9d8ecf2bd(drm/ttm: use per BO cleanup workers)My analysis:
two threads are running,
one in the irq signalling the fence, indma_fence_signal_timestamp_locked, it has done theDMA_FENCE_FLAG_SIGNALLED_BITsetting, but hasn't yet reached the callbacks.second thread in
nouveau_cli_work_ready, where it sees the fence is signalled, so then puts the fence, cleanups the object and frees the work item, which contains the callback.
Quoted 7 lines
thread one goes again and tries to call the callback and causes the use-after-free.
Proposed fix:
lock the fence signalled check innouveau_cli_work_ready, so either the callbacks are done or the memory is freed.Signed-off-by Dave Airlie <[email protected]>
deferred client work handler")
Is I think the most reasonable commit to tag to ensure backports.
Quoted 12 lines
--- drivers/gpu/drm/nouveau/nouveau_drm.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)drivers/gpu/drm/nouveau/nouveau_drm.c +0
-0@@ -137,10 +137,16 @@ nouveau_name(struct drm_device *dev)static inline bool nouveau_cli_work_ready (struct dma_fence *fence) {
drivers/gpu/drm/nouveau/nouveau_drm.c nouveau_name(struct drm_device *dev) · line 145
Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_drm.c +6
-4-if (!dma_fence_is_signaled (fence)) -return false; -dma_fence_put (fence); -return true; +unsigned long flags; +bool ret = true; +spin_lock_irqsave (fence->lock, flags); +if (!dma_fence_is_signaled_locked (fence)) + ret = false; +spin_unlock_irqrestore (fence->lock, flags);
I agree with the code being always broken as dma_fence_is_signaled_locked specifies: "This function requires &dma_fence.lock to be held."
Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_drm.c +4
-1+ +if (ret == true) +dma_fence_put (fence); +return ret; } static void --2.40.1
regardless of the patch fixing the issue users have seen:
Am Donnerstag, dem 15.06.2023 um 12:40 +1000 schrieb Dave Airlie:
Quoted 29 lines
From: Dave Airlie <[email protected]>
This seems to have existed for ever but is now more apparant after
9bff18d13473a9fdf81d5158248472a9d8ecf2bd(drm/ttm: use per BO cleanup workers)My analysis:
two threads are running,
one in the irq signalling the fence, indma_fence_signal_timestamp_locked, it has done theDMA_FENCE_FLAG_SIGNALLED_BITsetting, but hasn't yet reached the callbacks.second thread in
nouveau_cli_work_ready, where it sees the fence is signalled, so then puts the fence, cleanups the object and frees the work item, which contains the callback.thread one goes again and tries to call the callback and causes the use-after-free.
Proposed fix:
lock the fence signalled check innouveau_cli_work_ready, so either the callbacks are done or the memory is freed.Signed-off-by Dave Airlie <[email protected]>--- drivers/gpu/drm/nouveau/nouveau_drm.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)drivers/gpu/drm/nouveau/nouveau_drm.c +0
-0
drivers/gpu/drm/nouveau/nouveau_drm.c nouveau_name(struct drm_device *dev) · line 142
Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_drm.c +3
-4@@ -137,10 +137,16 @@ nouveau_name(struct drm_device *dev)static inline bool nouveau_cli_work_ready (struct dma_fence *fence) { -if (!dma_fence_is_signaled (fence)) -return false; -dma_fence_put (fence); -return true; +unsigned long flags; +bool ret = true; +spin_lock_irqsave (fence->lock, flags);
This function is only ever called from worker (process) context, so there is no point in saving the current irq state. This can be a plain spin_lock_irq.
Regards,
Lucas
Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_drm.c +7
-0+if (!dma_fence_is_signaled_locked (fence)) + ret = false; +spin_unlock_irqrestore (fence->lock, flags); + +if (ret == true) +dma_fence_put (fence); +return ret; } static void