Re: [PATCH] drm/fb-helper: defer setup when fbdev_probe() fails, not just on -EAGAIN

[Thomas Zimmermann]

Hi

Am 27.09.26 um 09:54 schrieb Nguyen Ngoc Thang:
> __drm_fb_helper_initial_config_and_unlock() calls
> drm_client_modeset_probe() first, which already fills in each
> mode_set->mode/num_connectors for every connected output, and only
> afterwards calls drm_fb_helper_single_fb_probe() to create
> fb_helper->fb and wire it into those mode_sets via
> drm_setup_crtcs_fb().
>
> If drm_fb_helper_single_fb_probe() fails with anything other than
> -EAGAIN (e.g. dev->driver->fbdev_probe() itself fails), the function
> bails out before drm_setup_crtcs_fb() runs and before setting
> fb_helper->deferred_setup. The client is left with mode_sets that
> have a mode and connectors but mode_set->fb == NULL, and nothing
> marks this fb_helper as needing a retry.
>
> __drm_fb_helper_restore_fbdev_mode_unlocked() only skips committing
> when fb_helper->deferred_setup is set, so a later .restore() call
> (drm_lastclose() on process exit, in this report) commits the stale
> mode_set through drm_client_modeset_commit() ->
> drm_client_modeset_commit_atomic() -> __drm_atomic_helper_set_config(),
> which hits WARN_ON(!set->fb).
>
> Mark fb_helper->deferred_setup on any drm_fb_helper_single_fb_probe()
> failure, not just -EAGAIN, so restore keeps skipping the commit until
> a later hotplug event successfully re-probes and re-populates
> mode_set->fb. The real error code is still returned to the caller
> unchanged.
>
> Reported-by: syzbot+ca23c8570669ead78867@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=ca23c8570669ead78867
> Fixes: ca91a2758fce ("drm/fb-helper: Support deferred setup")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>

Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

Thanks a lot for the fix.

Best regards
Thomas

> ---
> Tested in QEMU (KVM, x86_64) with the syzbot C reproducer, which enumerates
> a USB device over raw-gadget matching drm/gud's ID (idVendor 0x1d50,
> idProduct 0x614d) then opens the resulting DRM node. In our test VM gud
> lands on a different card minor than in the syzbot report since other
> drivers (vgem/vkms/bochs-drm) register first, but retargeting the
> reproducer's open() at gud's actual minor reproduces it directly on the
> very first probe:
>
>    gud 1-1:1.0: [drm] format XR24 little-endian (0x34325258) not supported
>    gud 1-1:1.0: [drm] No compatible format found
>    gud 1-1:1.0: [drm] *ERROR* fbdev: Failed to setup emulation (ret=-22)
>    ------------[ cut here ]------------
>    !set->fb
>    WARNING: drivers/gpu/drm/drm_atomic.c:2031 ...
>
> On the unpatched kernel this isn't benign: __drm_atomic_helper_set_config()
> warns and then continues, committing an active CRTC/plane with a NULL
> framebuffer, which escalates into a NULL-pointer dereference and a fatal
> kernel panic a few lines later in our runs. With this patch applied, the
> same gud probe failure (ret=-22) still happens every time as expected, but
> restore() no longer commits the stale mode_set: no warning, no panic,
> clean shutdown across repeated runs.
>
>   drivers/gpu/drm/drm_fb_helper.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d4664ed468b2..0817b639b3fa 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1724,10 +1724,10 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper)
>   
>   	ret = drm_fb_helper_single_fb_probe(fb_helper);
>   	if (ret < 0) {
> -		if (ret == -EAGAIN) {
> -			fb_helper->deferred_setup = true;
> +		/* modesets are probed but fb_helper->fb isn't; defer restore too */
> +		fb_helper->deferred_setup = true;
> +		if (ret == -EAGAIN)
>   			ret = 0;
> -		}
>   		mutex_unlock(&fb_helper->lock);
>   
>   		goto err_drm_fb_helper_release_info;

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)