Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/qxl: UAF in qxl_redraw_other_dumb_heads via lockless state access - [Medium] drm/qxl: Data race on dumb_shadow_needs_redraw causes missed redraws --
commit a0e42e8da93f570c9694adff24e9eb7f3e68dcda Author: Dillon Amburgey <[email protected]> drm/qxl: size packed dumb heads from the plane source This commit updates QXL to size and position dumb shadow buffers based on plane source rectangles instead of dumb BO allocations. It introduces a new flag to track redraw requirements and a function to recopy active heads during shadow reallocation. > diff --git a/drivers/gpu/drm/qxl/qxl_display.c > b/drivers/gpu/drm/qxl/qxl_display.c > index 0719fc6a52d53..d01504332bc83 100644 > --- a/drivers/gpu/drm/qxl/qxl_display.c > +++ b/drivers/gpu/drm/qxl/qxl_display.c [ ... ] > @@ -662,6 +703,37 @@ static void qxl_free_cursor(struct qxl_bo *cursor_bo) > qxl_bo_unref(&cursor_bo); > } > > +static void qxl_redraw_other_dumb_heads(struct qxl_device *qdev, int skip) > +{ > + struct drm_crtc *crtc; > + > + drm_for_each_crtc(crtc, &qdev->ddev) { > + struct drm_plane_state *st; > + struct qxl_bo *other; > + struct drm_clip_rect clip; > + u32 sx, sy; > + > + if (crtc->index == skip || > + !qdev->dumb_heads[crtc->index].width) > + continue; > + st = crtc->primary->state; > + if (!st->fb) > + continue; > + other = gem_to_qxl_bo(st->fb->obj[0]); [Severity: High] Does this lockless state access risk a use-after-free? When an atomic commit updates a dumb shadow buffer, qxl_primary_atomic_update() calls this function to loop over all CRTCs. If a concurrent atomic commit processes one of these other CRTCs, it could swap and free the old plane state and its framebuffer while they are being accessed here, because the iterating commit holds no modeset locks for these disjoint CRTCs. > + if (!other->is_dumb) > + continue; [ ... ] > @@ -670,13 +742,18 @@ static void qxl_primary_atomic_update(struct drm_plane > *plane, > struct qxl_device *qdev = to_qxl(plane->dev); > struct qxl_bo *bo = gem_to_qxl_bo(new_state->fb->obj[0]); > struct qxl_bo *primary; [ ... ] > qxl_draw_dirty_fb(qdev, new_state->fb, bo, 0, 0, &norect, 1, 1, > - dumb_shadow_offset); > + dumb_shadow_offset, y_off); > + if (qdev->dumb_shadow_needs_redraw) { > + qdev->dumb_shadow_needs_redraw = false; > + qxl_redraw_other_dumb_heads(qdev, new_state->crtc->index); > + } [Severity: Medium] Can this shared flag cause missed screen redraws due to a data race? The qdev->dumb_shadow_needs_redraw flag is shared globally across the device. When two atomic commits run concurrently on different CRTCs, both may set the flag to true during qxl_prepare_shadow(). The first commit to reach this update phase will clear the flag. The second commit will then see the flag as false and skip the required redraws for other heads. > } > > static void qxl_primary_atomic_disable(struct drm_plane *plane, [ ... ] > @@ -839,6 +918,7 @@ static void qxl_prepare_shadow(struct qxl_device *qdev, > struct qxl_bo *user_bo, > qxl_bo_create(qdev, surf.height * surf.stride, > true, true, QXL_GEM_DOMAIN_SURFACE, 0, > &surf, &qdev->dumb_shadow_bo); > + qdev->dumb_shadow_needs_redraw = true; [Severity: Medium] This is where the shared flag is set locklessly by potentially concurrent plane preparation steps. > } > if (user_bo->shadow != qdev->dumb_shadow_bo) { > if (user_bo->shadow) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
