Re: [PATCH] drm/atomic: remove bogus check for file_priv

On Wed, Jul 29, 2026 at 08:04:57AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Memory leak of `struct drm_pending_vblank_event` in `prepare_signaling()` error paths when `OUT_FENCE_PTR` is requested without `DRM_MODE_PAGE_FLIP_EVENT`.
> --
> 
> --- Patch [1]: [PATCH] drm/atomic: remove bogus check for file_priv ---
> commit 005f3f9bbee21ecd8f3b8b2687c9b0a8040286cc
> Author: Thadeu Lima de Souza Cascardo <[email protected]>
> 
> drm/atomic: remove bogus check for file_priv
> 
> This commit removes a file_priv check in prepare_signaling(), since
> drm_mode_atomic_ioctl() guarantees it is never NULL. Skipping the block
> here would cause a memory leak of drm_pending_vblank_event.
> 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -1445,9 +1445,6 @@ static int prepare_signaling(struct drm_device *dev,
> >  		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> >  			struct drm_pending_vblank_event *e = crtc_state->event;
> >  
> > -			if (!file_priv)
> > -				continue;
> > -
> >  			ret = drm_event_reserve_init(dev, file_priv, &e->base,
> >  						     &e->event.base);
> >  			if (ret) {
> 
> [Severity: High]
> This is a pre-existing issue, but can this function leak the
> drm_pending_vblank_event if an OUT_FENCE_PTR is requested without a
> DRM_MODE_PAGE_FLIP_EVENT?
> 
> If a user submits an atomic commit that requests an out-fence but does not
> request a page flip event, prepare_signaling() allocates an event:
> 
>     if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
>         ...
>         e = create_vblank_event(crtc, arg->user_data);
>         ...
>         crtc_state->event = e;
>     }
> 
> If the subsequent setup_out_fence() fails (e.g. from FD exhaustion), the
> function returns an error before setting event->base.fence:
> 
>     ret = setup_out_fence(&f[(*num_fences)++], fence);
>     if (ret) {
>         dma_fence_put(fence);
>         return ret;
>     }
>     
>     crtc_state->event->base.fence = fence;
> 
> In the cleanup path, complete_signaling() relies on event->base.fence or
> event->base.file_priv being set to free the event:
> 
>     if (event && (event->base.fence || event->base.file_priv)) {
>         drm_event_cancel_free(dev, &event->base);
>         crtc_state->event = NULL;
>     }
> 
> Since both fields are NULL, does the event get permanently leaked on this
> error path?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Fixed by https://lore.kernel.org/all/20260729-drm_pending_vblank_event_leak-v2-1-a5074aae07df@igalia.com/.

Cascardo.