[PATCH] drm/atomic: fix vblank event leak in complete_signaling()

[PATCH] drm/atomic: fix vblank event leak in complete_signaling()

[Abhishek Kumar]

When prepare_signaling() creates a vblank event via create_vblank_event()
but hits an error before the event is fully initialized (i.e. before
drm_event_reserve_init() sets file_priv or a fence is assigned to
event->base.fence), the subsequent call to complete_signaling() fails to
free the event because its cleanup condition requires at least one of
those fields to be set:

    if (event && (event->base.fence || event->base.file_priv))

This happens when only fence_ptr triggers event creation but a subsequent
allocation failure occurs before the fence is assigned to the event. The
128-byte event object is then orphaned and reported by kmemleak.

Fix this by adding an else-if branch that frees events which have no
completion callback set. Events allocated by
drm_atomic_helper_setup_commit() always have completion set, so checking
for its absence safely identifies events that were allocated by
prepare_signaling() but never fully set up.

Reported-by: syzbot+3fc9eecaf97147282c87@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3fc9eecaf97147282c87
Fixes: 92c715fca907 ("drm/atomic: Fix double free in drm_atomic_state_default_clear")
Cc: stable@vger.kernel.org
Signed-off-by: Abhishek Kumar <abhishek_sts8@yahoo.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 87de41fb4459..52a6b8436437 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1523,6 +1523,17 @@ static void complete_signaling(struct drm_device *dev,
 		if (event && (event->base.fence || event->base.file_priv)) {
 			drm_event_cancel_free(dev, &event->base);
 			crtc_state->event = NULL;
+		} else if (event && !event->base.completion) {
+			/*
+			 * The event was allocated by prepare_signaling()
+			 * but an error path was hit before the event got
+			 * fully set up (fence or file_priv assigned).
+			 * Events from drm_atomic_helper_setup_commit()
+			 * always have completion set, so checking for its
+			 * absence safely distinguishes our events.
+			 */
+			kfree(event);
+			crtc_state->event = NULL;
 		}
 	}
 
-- 
2.43.0

Claude review: drm/atomic: fix vblank event leak in complete_signaling()

[Claude Code Review Bot]

Overall Series Review

Subject: drm/atomic: fix vblank event leak in complete_signaling()
Author: Abhishek Kumar <abhishek_sts8@yahoo.com>
Patches: 1
Reviewed: 2026-03-31T17:48:23.896386

---

This is a single-patch fix for a real memory leak reported by syzbot. The analysis is correct: there is a window in `prepare_signaling()` where an event is allocated and assigned to `crtc_state->event`, but an error occurs before either `file_priv` or `fence` is set on it. The existing cleanup in `complete_signaling()` misses this case. The fix is reasonable and targeted.

**Verdict: The fix addresses a real bug, but the approach has a fragility concern worth discussing.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: fix vblank event leak in complete_signaling()

[Claude Code Review Bot]

Patch Review

**Bug analysis: Correct.** The leak path is:
1. `prepare_signaling()` calls `create_vblank_event()` → event allocated, `crtc_state->event = e` (line 1404)
2. `DRM_MODE_PAGE_FLIP_EVENT` not set → `drm_event_reserve_init()` skipped → `file_priv` remains NULL
3. `krealloc()` (line 1426) or `drm_crtc_create_fence()` (line 1436) fails → return before `event->base.fence` is assigned (line 1446)
4. `complete_signaling()` checks `event->base.fence || event->base.file_priv` → both NULL → event leaked

**The fix uses `!event->base.completion` as a discriminator:**

```c
} else if (event && !event->base.completion) {
    kfree(event);
    crtc_state->event = NULL;
}
```

This relies on the invariant that `drm_atomic_helper_setup_commit()` always sets `completion` (confirmed at `drm_atomic_helper.c:2563`), while `create_vblank_event()` uses `kzalloc` so `completion` starts as NULL.

**Concerns:**

1. **Fragility of the discriminator.** The correctness depends on a cross-file invariant: that `drm_atomic_helper_setup_commit()` always sets `completion` on events it touches, and that no other code path sets `completion` on a partially-initialized event. This is currently true, but it's an implicit contract. A comment in `drm_atomic_helper_setup_commit()` noting this dependency would help future maintainability.

2. **Why not fix `prepare_signaling()` directly?** The cleaner approach might be to have `prepare_signaling()` clean up its own partially-initialized events on error, before returning. For example, in the `fence_ptr` block, if `krealloc` or `drm_crtc_create_fence` fails, the code could do:
   ```c
   if (!f) {
       /* If event was created for fence_ptr only (no PAGE_FLIP_EVENT),
        * it hasn't been registered yet, so just kfree it */
       if (!(arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
           kfree(crtc_state->event);
           crtc_state->event = NULL;
       }
       return -ENOMEM;
   }
   ```
   This would keep cleanup co-located with allocation, which is a stronger pattern than relying on the `completion` field as a proxy for "who allocated this."

3. **The `kfree()` vs `drm_event_cancel_free()` distinction is correct.** Since the event was never passed to `drm_event_reserve_init()`, it was never added to the file's event list, so plain `kfree()` is the right call. `drm_event_cancel_free()` would try to manipulate a spinlock-protected list the event was never added to.

4. **Minor: commit message accuracy.** The commit message says "128-byte event object" — this is the size of `struct drm_pending_vblank_event`, which may vary by architecture/config. It's a minor nit but concrete byte counts can be misleading.

**Overall:** The patch fixes a real syzbot-reported leak and the logic is correct for the current codebase. However, I'd suggest the maintainers consider whether fixing the cleanup in `prepare_signaling()` itself (at each error return after event allocation) would be more robust than adding a heuristic discriminator in `complete_signaling()`. If the current approach is preferred, it should work correctly.

---
Generated by Claude Code Patch Reviewer