On Thu Jul 16, 2026 at 5:28 PM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
>
> New issues:
> - [Critical] Global state data race in `atomic_enable` leading to a NULL
> pointer dereference.
> - [High] Pending vblank events are leaked when the CRTC is disabled, causing
> userspace to hang.
>
> Pre-existing issues:
> - [High] Global state data race in `atomic_update` causing stolen events and
> userspace hangs.
> --
>
> --- Patch [8]: [PATCH v2 8/8] drm/xen: replace struct drm_simple_display_pipe
> with regular atomic helpers ---
> commit 545da3a98a8e32db9ed9bc44680620afafb3b601
> Author: Ze Huang <[email protected]>
>
> drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers
>
> Replace Xen frontend simple display pipe with explicit plane, CRTC and
> encoder objects for each pipeline.
>
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> b/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> @@ -105,24 +111,21 @@ static void send_pending_event(struct
>> xen_drm_front_drm_pipeline *pipeline)
>> spin_unlock_irqrestore(&dev->event_lock, flags);
>> }
>>
>> -static void display_enable(struct drm_simple_display_pipe *pipe,
>> - struct drm_crtc_state *crtc_state,
>> - struct drm_plane_state *plane_state)
>> +static void xen_drm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>> + struct drm_atomic_commit *commit)
>> {
>> - struct xen_drm_front_drm_pipeline *pipeline =
>> - to_xen_drm_pipeline(pipe);
>> - struct drm_crtc *crtc = &pipe->crtc;
>> + struct xen_drm_front_drm_pipeline *pipeline =
>> xen_drm_pipeline_from_crtc(crtc);
>> + struct drm_plane_state *plane_state = pipeline->plane.state;
>> + struct drm_framebuffer *fb = plane_state->fb;
>> int ret, idx;
>>
>> - if (!drm_dev_enter(pipe->crtc.dev, &idx))
>> + if (!drm_dev_enter(crtc->dev, &idx))
>> return;
>>
>> ret = xen_drm_front_mode_set(pipeline, crtc->x, crtc->y,
>> fb->width, fb->height,
>
> [Severity: Critical]
> Can this sequence cause a NULL pointer dereference?
>
> In xen_drm_crtc_helper_atomic_enable(), the global plane state is
> read asynchronously and fb is dereferenced directly.
>
> If userspace submits two non-blocking atomic commits rapidly (the
> first enabling the CRTC and plane, and the second disabling the plane),
> the global state pointer (pipeline->plane.state) is swapped
> synchronously during the ioctl.
>
> When the first commit's worker executes, it reads the newer state where
> fb is NULL, and unconditionally dereferences fb->width.
>
> Would it be safer to extract the state from the commit object using
> drm_atomic_get_new_plane_state(commit, &pipeline->plane)?
As replied in patch 5, will do
>
>> fb->format->cpp[0] * 8,
>> xen_drm_front_fb_to_cookie(fb));
>> -
>> if (ret) {
>> DRM_ERROR("Failed to enable display: %d\n", ret);
>
> [ ... ]
>
>> @@ -131,13 +134,13 @@ static void display_enable(struct
>> drm_simple_display_pipe *pipe,
>> drm_dev_exit(idx);
>> }
>>
>> -static void display_disable(struct drm_simple_display_pipe *pipe)
>> +static void xen_drm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> + struct drm_atomic_commit *commit)
>> {
>> - struct xen_drm_front_drm_pipeline *pipeline =
>> - to_xen_drm_pipeline(pipe);
>> + struct xen_drm_front_drm_pipeline *pipeline =
>> xen_drm_pipeline_from_crtc(crtc);
>> int ret = 0, idx;
>>
>> - if (drm_dev_enter(pipe->crtc.dev, &idx)) {
>> + if (drm_dev_enter(crtc->dev, &idx)) {
>> ret = xen_drm_front_mode_set(pipeline, 0, 0, 0, 0, 0,
>> xen_drm_front_fb_to_cookie(NULL));
>> drm_dev_exit(idx);
>> @@ -147,6 +150,11 @@ static void display_disable(struct
>> drm_simple_display_pipe *pipe)
>>
>> /* Make sure we can restart with enabled connector next time */
>> pipeline->conn_connected = true;
>> +
>> + /* release stalled event if any */
>> + send_pending_event(pipeline);
>> +}
>
> [Severity: High]
> Does this function leak the vblank event when the CRTC is disabled?
>
> When moving away from drm_simple_kms_helper, the driver loses the core
> helper's automatic consumption of crtc_state->event during CRTC
> disable.
>
> If userspace submits an atomic commit that disables the CRTC and
> requests a vblank event (e.g., modeset to 0 or DPMS off),
> xen_drm_crtc_helper_atomic_disable() completely omits the consumption and
> signaling of the event.
>
> This leaves the event dangling, which causes a kernel warning in the
> commit tail and hangs the userspace compositor indefinitely as it waits
> for the pageflip event.
>
> Should this function pull and signal the event from
> drm_atomic_get_new_crtc_state(commit, crtc)->event?
>
I would like to preserve the original xen frontend behavior here.
> [ ... ]
>
>> @@ -242,12 +261,12 @@ static int display_check(struct
>> drm_simple_display_pipe *pipe,
>> return 0;
>> }
>>
>> -static void display_update(struct drm_simple_display_pipe *pipe,
>> - struct drm_plane_state *old_plane_state)
>> +static void xen_drm_plane_helper_atomic_update(struct drm_plane *plane,
>> + struct drm_atomic_commit *commit)
>> {
>> - struct xen_drm_front_drm_pipeline *pipeline =
>> - to_xen_drm_pipeline(pipe);
>> - struct drm_crtc *crtc = &pipe->crtc;
>> + struct drm_plane_state *old_plane_state =
>> drm_atomic_get_old_plane_state(commit, plane);
>> + struct xen_drm_front_drm_pipeline *pipeline =
>> xen_drm_pipeline_from_plane(plane);
>> + struct drm_crtc *crtc = &pipeline->crtc;
>> struct drm_pending_vblank_event *event;
>> int idx;
>
> [Severity: High]
> This is a pre-existing issue, but can this code steal events from
> future commits?
>
> In xen_drm_plane_helper_atomic_update(), the global CRTC state's event
> is accessed directly:
>
> event = crtc->state->event;
>
> Since crtc->state is swapped synchronously, it may reflect a future
> commit's state by the time this worker runs. The worker would then steal
> the future commit's event and clear it.
>
> When the future commit's worker runs, it finds no event, while the
> current commit's original event is stranded on its old state object and
> never signaled, resulting in userspace hangs.
>
> Should the event be fetched reliably from the specific commit object
> using drm_atomic_get_new_crtc_state(commit, crtc)->event?
Will access crtc state with drm_atomic_get_new_crtc_state(commit, crtc)
