Re: [PATCH] drm/vblank: Don't arm vblank timer with invalid frame duration

[Thomas Zimmermann <tzimmermann@suse.de>]

(cc Ville)

Hi,

thanks for addressing the issue.

Am 14.06.26 um 00:44 schrieb Roman Ilin:
> When a CRTC's display mode carries a too small pixel clock,
> drm_calc_timestamping_constants() computes a frame duration that
> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
> interval, after which vblank events are no longer delivered. Pending
> page flips never complete and the display appears frozen.
>
> This could be triggered on virtio-gpu guests that have dynamic resolution
> enabled: when the SPICE agent or the X server resizes the output, it
> submits a mode whose pixel clock is off by a factor of 1000, e.g.:

'off by' as in it should be in Hz rather than kHz.

>
>      clock = 406 kHz, htotal = 3152, vtotal = 2148
>
>      framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
>
> 16675852216 does not fit into an int and wraps to roughly -504000000.
> ns_to_ktime() then yields a negative interval and the timer stops working.
>
> Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
> Use vblank timer"). That commit merely made virtio-gpu use the vblank
> timer and thereby exposed the pre-existing problem in the timer setup
> added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
>
> Reject a non-positive frame duration in drm_crtc_vblank_start_timer() and
> return an error. enable_vblank then fails and the driver falls back to
> sending the vblank event immediately, as it did before the vblank timer
> was introduced. Valid modes are unaffected, and the timer self-heals on
> the next mode that has a sane clock.
>
> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
> Cc: stable@vger.kernel.org
> Signed-off-by: Roman Ilin <me@romanilin.is>
> ---
> Notes:
>
> Based on v7.1-rc7. Tested on 6.19 and 7.1-rc7.
>
> Open questions:
>
> This relies on the int overflow producing a negative value. The deeper
> issue is that drm_calc_timestamping_constants() truncates framedur_ns to
> int. Would you prefer to widen framedur_ns to s64, or to bound the
> interval here (e.g. reject framedur_ns above one second) so that any
> bogus interval is rejected regardless of sign?

Generally speaking, I think we should not accept such a bugos mode in 
the first place. But this would require changes to the mode-setting code 
that are too invasive for a bug fix.

So, if anything, we should try to detect the problem in 
drm_calc_timestamping_constants(). Let's make the helper return errno 
codes instead of failing silently. Within the helper, let's do the 
following changes:

- declare frame_size an unsigned int
- declare linedur_ns and framedur_ns of type u64

This should avoid possible overflows in the code.  And before assigning 
linedur_ns and framedur_ns to the vblank fields, test them against INT_MAX.

Maybe at [1]. Using drm_WARN_ON_ONCE is likely a good idea for future 
debugging.

if (drm_WARN_ON_ONCE(dev, linedur_ns > INT_MAX) || drm_WARN_ON_ONCE(dev, 
framedur_ns > INT_MAX))
    return -EINVAL

[1] 
https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/drm_vblank.c#L662


>
> Should virtio-gpu additionally sanitize the user-supplied clock in its
> atomic_check (similar to vmwgfx for the clock==0 case) so the
> vblank-timer throttling is preserved for these resizes, instead of
> falling back to immediate events?
>
>   drivers/gpu/drm/drm_vblank.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f78bf37f1..557cd0bc8 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -2235,7 +2235,13 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
>   
>   	drm_calc_timestamping_constants(crtc, &crtc->mode);
>   
> +	/*
> +	 * Return an error so the driver falls back to sending vblank events
> +	 * when a small mode clock yields a frame duration exceeding INT_MAX.
> +	 */
> +	if (vblank->framedur_ns <= 0)
> +		return -EINVAL;

Here, you would just forward the error upwards in the call stack.

Best regards
Thomas

> +
>   	spin_lock_irqsave(&vtimer->interval_lock, flags);
>   	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
>   	spin_unlock_irqrestore(&vtimer->interval_lock, flags);

-- 
--
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)