* [PATCH] drm/atomic: fix vblank event leak in complete_signaling() [not found] <20260329073423.8390-1-abhishek_sts8.ref@yahoo.com> @ 2026-03-29 7:34 ` Abhishek Kumar 2026-03-31 7:48 ` Claude review: " Claude Code Review Bot 2026-03-31 7:48 ` Claude Code Review Bot 0 siblings, 2 replies; 3+ messages in thread From: Abhishek Kumar @ 2026-03-29 7:34 UTC (permalink / raw) To: dri-devel, linux-kernel Cc: maarten.lankhorst, mripard, tzimmermann, airlied, simona, syzbot+3fc9eecaf97147282c87, stable, 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Claude review: drm/atomic: fix vblank event leak in complete_signaling() 2026-03-29 7:34 ` [PATCH] drm/atomic: fix vblank event leak in complete_signaling() Abhishek Kumar @ 2026-03-31 7:48 ` Claude Code Review Bot 2026-03-31 7:48 ` Claude Code Review Bot 1 sibling, 0 replies; 3+ messages in thread From: Claude Code Review Bot @ 2026-03-31 7:48 UTC (permalink / raw) To: dri-devel-reviews 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Claude review: drm/atomic: fix vblank event leak in complete_signaling() 2026-03-29 7:34 ` [PATCH] drm/atomic: fix vblank event leak in complete_signaling() Abhishek Kumar 2026-03-31 7:48 ` Claude review: " Claude Code Review Bot @ 2026-03-31 7:48 ` Claude Code Review Bot 1 sibling, 0 replies; 3+ messages in thread From: Claude Code Review Bot @ 2026-03-31 7:48 UTC (permalink / raw) To: dri-devel-reviews 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-03-31 7:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260329073423.8390-1-abhishek_sts8.ref@yahoo.com>
2026-03-29 7:34 ` [PATCH] drm/atomic: fix vblank event leak in complete_signaling() Abhishek Kumar
2026-03-31 7:48 ` Claude review: " Claude Code Review Bot
2026-03-31 7:48 ` Claude Code Review Bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox