Hi
Am 22.06.26 um 11:47 schrieb Icenowy Zheng:
在 2026-06-22一的 11:11 +0200，Thomas Zimmermann写道：
Hi
Am 19.06.26 um 18:59 schrieb Icenowy Zheng:
在 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.
Yes, this patch looks okay, but I wonder whether the crtc index
query
should be done in drm_client_modeset_wait_for_vblank() instead.
There's another call to this helper from the ioctl helper. It has a
lengthy comment about using CRTC 0 only and how these indices don't
make
much sense with fbdev. I'll look into generalizing the code a bit,
I think both codepaths are based on the same (wrong) assumption that
CRTC 0 is always the first used CRTC, so that codepath should also be
fixed; integrating the logic into drm_client_modeset_wait_for_vblank()
will not affect drivers that use CRTC 0 first, but will make drivers
that have fixed CRTC preference work with FBIO_WAITFORVSYNC too (which
currently do not work).
DRM's fbdev emulation is legacy code and most likely not used besides
the kernel's console. Until we have a bug reports against the ioctl,
there nothing to fix here.
Interfaces for drm_client_ are supposed to work with any client.
Integrating that look-up code there might make other clients unhappy.
Best regards
Thomas
Thanks,
Icenowy
but
I'd rather not fully integrate the logic into
drm_client_modeset_wait_for_vblank().
Best regards
Thomas
I will test this patch this weekend.
Thanks,
Icenow
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
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
