在 2026-06-19五的 10:50 +0200，Thomas Zimmermann写道： > (cc Icenowy) > > Hi, > > there are a number of similar reports. Does the attached patch fix > the > problem. It picks up what you do, but in a more correct way.
Well in fact I think the correct practice might be combining both -- use crtc index in modeset + skip non-enabled CRTCs. But anyway the attached patch is tested by me on loongson driver, if you're going to send it, feel free to add: ``` Tested-by: Icenowy Zheng <[email protected]> ``` In addition, a bug report with loongson driver exists on the Debian bugtracker, the Debian kernel team wants the fix to have: ``` Closes: https://bugs.debian.org/1138033 ``` Thanks, Icenowy > > Best regards > Thomas > > Am 18.06.26 um 19:56 schrieb H. Nikolaus Schaller: > > > > > Am 18.06.2026 um 19:43 schrieb [email protected]: > > > > > > Thank you for your contribution! Sashiko AI review found 3 > > > potential issue(s) to consider: > > > - [High] Lockless access to `crtc->state` and `crtc->primary- > > > >state` can lead to a Use-After-Free due to concurrent atomic > > > commits. > > > - [High] The patch breaks vblank synchronization for legacy (non- > > > atomic) DRM drivers. > > > - [Medium] The logic selects the first matching CRTC instead of > > > the last active one, contradicting the commit message. > > > -- > > > > > > commit 0337b26b606b4d905d72ea2be31bcb984fe62c6e > > > Author: H. Nikolaus Schaller <[email protected]> > > > > > > drm/fb_helper: Determine proper crtc_index within > > > drm_fb_helper_fb_dirty > > > > > > This commit updates the dirty framebuffer worker to search for a > > > matching > > > active CRTC instead of hardcoding the wait for vblank to CRTC > > > index 0. It > > > aims to avoid vblank timeouts when CRTC 0 is inactive. > > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > > > b/drivers/gpu/drm/drm_fb_helper.c > > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > [ ... ] > > > > @@ -232,9 +232,23 @@ static void drm_fb_helper_fb_dirty(struct > > > > drm_fb_helper *helper) > > > > struct drm_clip_rect clip_copy; > > > > unsigned long flags; > > > > int ret; > > > > + unsigned int crtc_index = 0; > > > > + struct drm_crtc *crtc; > > > > > > > > mutex_lock(&helper->lock); > > > > - drm_client_modeset_wait_for_vblank(&helper->client, 0); > > > > + > > > > + drm_for_each_crtc(crtc, dev) { > > > > + if (crtc->primary && crtc->primary->state && > > > > + crtc->primary->state->fb == helper->fb && > > > > + crtc->state && crtc->state->active) { > > > [Severity: High] > > > Can this lockless access to crtc->state and crtc->primary->state > > > result in a > > > use-after-free? > > No idea... > > > > > The drm_fb_helper_fb_dirty() function holds helper->lock, but > > > does not hold > > > the DRM modeset locks required to protect these state pointers. > > > During a > > Which locks are exactly recommended to add? What is their > > name/pointer? > > > > > concurrent atomic modeset commit, the DRM core allocates new > > > state objects, > > > swaps them into place, and frees the old objects via kfree(). > > > > > > If this code reads an old state pointer and gets preempted, could > > > it then > > > dereference the pointer after it has been freed by the concurrent > > > commit? > > > > > > [Severity: High] > > > Does this break vblank synchronization for legacy non-atomic DRM > > > drivers? > > > Legacy drivers do not use the atomic modesetting infrastructure, > > > so their > > > crtc->state and crtc->primary->state pointers remain NULL. Since > > > the condition > > > above requires these pointers to be non-NULL, the loop will skip > > > the vblank > > > wait entirely for legacy drivers. This skips the intended rate- > > > limiting > > > behavior and could lead to unbounded damage worker execution. > > What is the solution? And is this important. > > > > Before d8c4bddcd8bcb ("drm/fb-helper: Synchronize dirty worker with > > vblank") > > > > there was also no rate-limiting behavior. > > > > > > + crtc_index = drm_crtc_index(crtc) + 1; > > > > + break; > > > [Severity: Medium] > > > The commit message states "If multiple CRTCs match, the last > > > active one is > > > selected." Does the break statement here cause the first active > > > CRTC to be > > > selected instead, bypassing the remaining matches in the list? > > Yes, that is correct. The description is indeed wrong. > > > > > > + } > > > > + } > > > > + > > > > + if (crtc_index > 0) > > > > + drm_client_modeset_wait_for_vblank(&helper->client, > > > > crtc_index - 1); > > > > + > > > > mutex_unlock(&helper->lock); > > > -- > > > Sashiko AI review · > > > https://sashiko.dev/#/patchset/5118b37e579cec220f2d2b6ab0bb1ecfb67f0d5c.1781803617.git....@goldelico.com?part=1
