Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] Moving `nvif_event_dtor()` before `nouveau_fence_context_kill()` introduces an unprotected Time-of-Check to Time-of-Use (TOCTOU) race condition that results in a NULL pointer dereference in `nvif_object_ioctl`. --
--- Patch [1]: [PATCH 1/3] drm/nouveau: destroy the fence event before cancelling its work --- commit 248f9b5f2479dc65d3572babf739eb3883d7b9f7 Author: Marek Czernohous <[email protected]> drm/nouveau: destroy the fence event before cancelling its work This commit reorders the teardown sequence in nouveau_fence_context_del() by moving nvif_event_dtor() before cancel_work_sync() and nouveau_fence_context_kill(). It aims to prevent a newly arriving interrupt from re-arming the work that was just cancelled. Link: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c > b/drivers/gpu/drm/nouveau/nouveau_fence.c > index edbe9e08ba0fd..4a3698dc2cd14 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -96,9 +96,9 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, > int error) > void > nouveau_fence_context_del(struct nouveau_fence_chan *fctx) > { > + nvif_event_dtor(&fctx->event); > cancel_work_sync(&fctx->uevent_work); > nouveau_fence_context_kill(fctx, 0); [Severity: Critical] Does moving nvif_event_dtor() before nouveau_fence_context_kill() introduce a Time-of-Check to Time-of-Use (TOCTOU) race condition? Since nouveau_fence_context_kill() hasn't run yet, the fences are still active and unsignaled. If Thread A is polling a fence and calls nouveau_fence_enable_signaling() which reaches nvif_event_allow(), it might evaluate nvif_event_constructed() right before Thread B destroys the event: Thread A: drivers/gpu/drm/nouveau/nvif/event.c:nvif_event_allow() { if (nvif_event_constructed(event)) { Thread B concurrently cleans up the channel: drivers/gpu/drm/nouveau/nouveau_fence.c:nouveau_fence_context_del() { nvif_event_dtor(&fctx->event); ... drivers/gpu/drm/nouveau/nvif/object.c:nvif_object_dtor() { nvif_object_ioctl(object, &args, sizeof(args), NULL); object->client = NULL; Thread A then continues and calls into nvif_mthd(): drivers/gpu/drm/nouveau/nvif/event.c:nvif_event_allow() { int ret = nvif_mthd(&event->object, NVIF_EVENT_V0_ALLOW, ...); ... drivers/gpu/drm/nouveau/nvif/object.c:nvif_object_ioctl() { struct nvif_client *client = object->client; /* ... */ return client->driver->ioctl(client->object.priv, data, size, hack); Could Thread A read the stale object->client as NULL and fatally dereference it here, since both polling on an exported sync_file fd and channel cleanup are accessible to unprivileged userspace? > - nvif_event_dtor(&fctx->event); > fctx->dead = 1; > > /* -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
