> 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
