On Wed, Jul 29, 2026 at 06:23:42AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
>
> Pre-existing issues:
> - [Medium] API Contract Violation: out_fence_ptr is not set to -1 on certain
> error paths
> --
>
> --- Patch [1]: [PATCH v2] drm: Fix drm_pending_vblank_event leak in error
> path for out_fence_ptr ---
> commit ee648481e8c7946df39aedd0d40de2f6f6522bf2
> Author: Thadeu Lima de Souza Cascardo <[email protected]>
>
> drm: Fix drm_pending_vblank_event leak in error path for out_fence_ptr
>
> This commit fixes a memory leak in the error path of prepare_signaling when
> DRM_MODE_PAGE_FLIP_EVENT is not set but an out_fence_ptr is provided. It
> ensures the pending vblank event is properly released and the crtc state
> event pointer is cleared on allocation or fence creation failures.
>
> Link:
> https://patch.msgid.link/20260728-drm_pending_vblank_event_leak-v1-1-08429b920...@igalia.com
>
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 1050dddadb17e..aed6040182eb6 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -1464,11 +1464,15 @@ static int prepare_signaling(struct drm_device *dev,
> > if (fence_ptr) {
> > struct dma_fence *fence;
> > struct drm_out_fence_state *f;
> > + struct drm_pending_vblank_event *e = crtc_state->event;
> >
> > f = krealloc(*fence_state, sizeof(**fence_state) *
> > (*num_fences + 1), GFP_KERNEL);
> > - if (!f)
> > + if (!f) {
> > + drm_event_cancel_free(dev, &e->base);
> > + crtc_state->event = NULL;
> > return -ENOMEM;
> > + }
>
> [Severity: Medium]
> This is a pre-existing issue, but I noticed a potential UAPI contract
> violation here.
>
> If krealloc fails, we return -ENOMEM before out_fence_ptr is added
> to the fence_state array or num_fences is incremented.
>
> Does this mean complete_signaling() will skip this element and fail to write
> -1 back to the userspace out_fence_ptr, leaving userspace with an
> uninitialized or stale fd value?
>
This is set to -1 at drm_atomic_crtc_set_property().
> >
> > memset(&f[*num_fences], 0, sizeof(*f));
> >
> > @@ -1476,12 +1480,17 @@ static int prepare_signaling(struct drm_device *dev,
> > *fence_state = f;
> >
> > fence = drm_crtc_create_fence(crtc);
> > - if (!fence)
> > + if (!fence) {
> > + drm_event_cancel_free(dev, &e->base);
> > + crtc_state->event = NULL;
> > return -ENOMEM;
> > + }
>
> [Severity: Medium]
> This is also a pre-existing issue, but similarly, if drm_crtc_create_fence
> fails, out_fence_ptr is written to the array but num_fences is not
> incremented.
>
> Will the cleanup routine skip this pointer and fail to write -1 to userspace?
>
Same as above.
> It looks like this same pattern also exists in the connector loop later in
> prepare_signaling() when drm_writeback_get_out_fence() fails.
>
Same for set_out_fence_for_connector(). As for the connector loop, I don't
see such potential leaks there.
Regards.
Cascardo.
> >
> > ret = setup_out_fence(&f[(*num_fences)++], fence);
> > if (ret) {
> > dma_fence_put(fence);
> > + drm_event_cancel_free(dev, &e->base);
> > + crtc_state->event = NULL;
> > return ret;
> > }
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/20260729-drm_pending_vblank_event_leak-v2-1-a5074aae0...@igalia.com?part=1
