From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/atomic: fix vblank event leak in complete_signaling() Date: Tue, 31 Mar 2026 17:48:24 +1000 Message-ID: In-Reply-To: <20260329073423.8390-1-abhishek_sts8@yahoo.com> References: <20260329073423.8390-1-abhishek_sts8@yahoo.com> <20260329073423.8390-1-abhishek_sts8@yahoo.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Bug analysis: Correct.** The leak path is: 1. `prepare_signaling()` calls `create_vblank_event()` =E2=86=92 event allo= cated, `crtc_state->event =3D e` (line 1404) 2. `DRM_MODE_PAGE_FLIP_EVENT` not set =E2=86=92 `drm_event_reserve_init()` = skipped =E2=86=92 `file_priv` remains NULL 3. `krealloc()` (line 1426) or `drm_crtc_create_fence()` (line 1436) fails = =E2=86=92 return before `event->base.fence` is assigned (line 1446) 4. `complete_signaling()` checks `event->base.fence || event->base.file_pri= v` =E2=86=92 both NULL =E2=86=92 event leaked **The fix uses `!event->base.completion` as a discriminator:** ```c } else if (event && !event->base.completion) { kfree(event); crtc_state->event =3D 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-f= ile invariant: that `drm_atomic_helper_setup_commit()` always sets `complet= ion` 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 implici= t contract. A comment in `drm_atomic_helper_setup_commit()` noting this dep= endency would help future maintainability. 2. **Why not fix `prepare_signaling()` directly?** The cleaner approach mig= ht be to have `prepare_signaling()` clean up its own partially-initialized = events on error, before returning. For example, in the `fence_ptr` block, i= f `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 =3D 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 allocate= d this." 3. **The `kfree()` vs `drm_event_cancel_free()` distinction is correct.** S= ince 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 ev= ent object" =E2=80=94 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 c= orrect for the current codebase. However, I'd suggest the maintainers consi= der whether fixing the cleanup in `prepare_signaling()` itself (at each err= or return after event allocation) would be more robust than adding a heuris= tic discriminator in `complete_signaling()`. If the current approach is pre= ferred, it should work correctly. --- Generated by Claude Code Patch Reviewer