diff mbox series

[5/5] drm/vblank: Require all CRTCs implement vblank support in drm_vblank_init()

Message ID 20260814203542.1405135-6-lyude@redhat.com (mailing list archive)
State New
Headers
Series drm/vblank: Enforce all-or-nothing vblank support |

Commit Message

Lyude Paul Aug. 14, 2026, 8:35 p.m. UTC 
Due to us previously allowing users to specify the number of CRTCs for
drm_vblank_init() manually, a untested and almost certainly UB edge case
was technically possible for drivers: implementing vblank support for some
of their CRTCs, and not implementing it for others.

I don't think there's any actual situation in the real world where hardware
like this exists, nor do I think supporting such a possibility was ever
actually the original intent here. But in Rust, we need to be able to
prevent such a situation from occurring since we cannot guarantee no UB is
possible otherwise. I think it makes sense here for us to just handle this
safety check in the DRM core though, since it's quite likely such an edge
case would break C drivers in addition to Rust drivers.

So, let's start enforcing this in drm_vblank_init by checking that every
CRTC has an enable_vblank function registered. Throw a warning if we find
that some CRTCs have it and others don't.

In the event that we don't actually find any CRTCs with vblank support,
simply skip init and return -ENODEV. We intentionally don't WARN_ON() this
scenario, as it isn't a fatal error and drivers which do support KMS but
don't end up registering any CRTCs can simply ignore the error. This is
also useful for Rust, since it saves us from having to track whether or not
any CRTCs registered vblank support ourselves.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/drm_vblank.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

sashiko-bot@kernel.org Aug. 14, 2026, 9:08 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm_vblank_init() causes a NULL pointer dereference or driver probe failure due to accessing uninitialized or empty crtc_list
--

--- Patch [5]: [PATCH 5/5] drm/vblank: Require all CRTCs implement vblank support in drm_vblank_init() ---
commit 178bbb448a16663eaad60425c62b446801e0f387
Author: Lyude Paul <lyude@redhat.com>

drm/vblank: Require all CRTCs implement vblank support in drm_vblank_init()

This commit modifies drm_vblank_init() to verify that all CRTCs implement
vblank support, ensuring an all-or-nothing configuration. It returns -EINVAL
if support is mixed, and -ENODEV if no CRTCs support vblank.

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index d57405050554f..17affae23e7a6 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -542,12 +542,31 @@ static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
>   * drmm_add_action_or_reset().
>   *
>   * Returns:
> - * Zero on success or a negative error code on failure.
> + * Zero on success or a negative error code on failure. If no CRTCs with vblank
> + * support implemented were found, -ENODEV will be returned.
>   */
>  int drm_vblank_init(struct drm_device *dev)
>  {
>  	int ret;
>  	struct drm_crtc *crtc;
> +	bool found_vbl_crtc = false, found_non_vbl_crtc = false;
> +
> +	/* Vblank hooks are per-CRTC, not per-device - which is contrary to how
> +	 * the vblank core was written. Since hardware that only has vblank
> +	 * support on some CRTCs but not all is non-existent and would likely
> +	 * lead to UB, ensure that vblank support is all or nothing.
> +	 */
> +	drm_for_each_crtc(crtc, dev) {

[Severity: High]
Can iterating over dev->mode_config.crtc_list here cause a NULL pointer
dereference or a false -ENODEV return during driver probe?

Several DRM drivers call drm_vblank_init() before initializing mode_config
or creating CRTCs. For example:

In i915, intel_display_driver_probe_noirq() calls drm_vblank_init() before
intel_mode_config_init() sets up the crtc_list, meaning the list head is
uninitialized.

In vkms, vkms_create() calls drm_vblank_init() before vkms_modeset_init()
initializes the crtc_list.

In zynqmp_kms, zynqmp_dpsub_drm_init() initializes the list but calls
drm_vblank_init() before any CRTCs are created, meaning the list is empty
and this function will unconditionally return -ENODEV.

Does drm_vblank_init() need to support being called before CRTCs are
registered?

> +		if (crtc->funcs->enable_vblank)
> +			found_vbl_crtc = true;
> +		else
> +			found_non_vbl_crtc = true;
> +
> +		if (drm_WARN_ON(dev, found_vbl_crtc && found_non_vbl_crtc))
> +			return -EINVAL;
> +	}
> +	if (!found_vbl_crtc)
> +		return -ENODEV;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index d57405050554f..17affae23e7a6 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -542,12 +542,31 @@  static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
  * drmm_add_action_or_reset().
  *
  * Returns:
- * Zero on success or a negative error code on failure.
+ * Zero on success or a negative error code on failure. If no CRTCs with vblank
+ * support implemented were found, -ENODEV will be returned.
  */
 int drm_vblank_init(struct drm_device *dev)
 {
 	int ret;
 	struct drm_crtc *crtc;
+	bool found_vbl_crtc = false, found_non_vbl_crtc = false;
+
+	/* Vblank hooks are per-CRTC, not per-device - which is contrary to how
+	 * the vblank core was written. Since hardware that only has vblank
+	 * support on some CRTCs but not all is non-existent and would likely
+	 * lead to UB, ensure that vblank support is all or nothing.
+	 */
+	drm_for_each_crtc(crtc, dev) {
+		if (crtc->funcs->enable_vblank)
+			found_vbl_crtc = true;
+		else
+			found_non_vbl_crtc = true;
+
+		if (drm_WARN_ON(dev, found_vbl_crtc && found_non_vbl_crtc))
+			return -EINVAL;
+	}
+	if (!found_vbl_crtc)
+		return -ENODEV;
 
 	spin_lock_init(&dev->vbl_lock);
 	spin_lock_init(&dev->vblank_time_lock);