Re: [PATCH v2 8/8] drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

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 <ze.huang@oss.qualcomm.com>

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)?

>  				     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?

[ ... ]

> @@ -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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=8