Hi
Am 04.07.26 um 20:45 schrieb [email protected]:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
to consider:
New issues:
- [High] Race condition and potential NULL pointer dereference due to direct
access to `plane->state` and missing visibility checks in atomic callbacks.
Pre-existing issues:
- [High] Sleeping in atomic context within CRTC and plane atomic callbacks.
--
commit a10b3d5781e3b7143f432c86643ef47e8ddc3a9a
Author: Ze Huang <[email protected]>
drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers
Convert gm12u320 to direct primary plane, CRTC and encoder setup.
Keep shadow-plane helper state, framebuffer access helpers and
no-scaling plane-state check from simple-KMS path.
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 4ad074337af07..b476793d0401e 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
[ ... ]
+static void gm12u320_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *commit)
{
struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT };
- struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
+ struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
+ struct drm_plane_state *plane_state = crtc->primary->state;
[Severity: High]
Will this cause a NULL pointer dereference if a subsequent commit disables
the plane?
Primary is for (old) non-atomic DRM drivers.
Because non-blocking commits execute asynchronously, crtc->primary->state
reflects the state of the most recently queued commit, rather than the
commit currently executing. If a fast subsequent commit disables the plane,
plane_state->fb could be NULL.
Should this use drm_atomic_get_new_plane_state(commit, crtc->primary) to
get the state tied to this specific commit?
No. Changes that require plane state should go into the plane's
atomic_enable or atomic_update.
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT;
gm12u320_fb_mark_dirty(plane_state->fb, &shadow_plane_state->data[0],
&rect);
}
-static void gm12u320_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void gm12u320_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit
*commit)
{
- struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
+ struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
gm12u320_stop_fb_update(gm12u320);
[Severity: High]
This isn't a bug introduced by this patch, but does gm12u320_stop_fb_update()
sleep in atomic context?
This is a false positive.
Best regards
Thomas
gm12u320_stop_fb_update() calls cancel_delayed_work_sync() and mutex_lock().
Since CRTC atomic disable callbacks can run in atomic context during
non-blocking commits, sleeping here is forbidden and could cause kernel
warnings or deadlocks.
}
-static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *old_state)
+static void gm12u320_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit
*commit)
{
- struct drm_plane_state *state = pipe->plane.state;
+ struct drm_plane_state *old_state =
drm_atomic_get_old_plane_state(commit, plane);
+ struct drm_plane_state *state = plane->state;
[Severity: High]
Could accessing plane->state directly here lead to a crash?
Similar to the CRTC enable function, reading plane->state directly instead of
using drm_atomic_get_new_plane_state(commit, plane) accesses the future
state.
If a subsequent commit disables the plane, state->fb will be NULL. In this
scenario, drm_atomic_helper_damage_merged() returns true for plane
disablements, leading to gm12u320_fb_mark_dirty() being called with a NULL
fb, which will oops when trying to dereference fb->dev.
This wasn't possible before because the drm_simple_kms_helper explicitly
checked if (!plane->state->visible) before calling the update hook, but
that protection is dropped in this conversion.
+ struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(state);
struct drm_rect rect;
- if (drm_atomic_helper_damage_merged(old_state, state, &rect))
+ if (drm_atomic_helper_damage_merged(old_state, state, &rect))
gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->data[0],
&rect);
[Severity: High]
This is a pre-existing issue, but does gm12u320_fb_mark_dirty() sleep in
atomic context?
It calls mutex_lock(&gm12u320->fb_update.lock), which is unsafe to call
from a plane update callback during non-blocking commits, as these run
in atomic context.
}
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
