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

[Claude Code Review Bot <claude-review@example.com>]

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