diff mbox series

[4/5] drm/vblank: Use drm_for_each_crtc() in drm_vblank_init()

Message ID 20260814203542.1405135-5-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 
No functional changes yet, we just should be using this iterator here
instead of manually counting the number of CRTCs anyhow.

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

Comments

sashiko-bot@kernel.org Aug. 14, 2026, 8:56 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Calling `drm_vblank_init` before creating CRTCs results in a zero-size allocation, leading to a kernel crash when vblank features are later used.
--

--- Patch [4]: [PATCH 4/5] drm/vblank: Use drm_for_each_crtc() in drm_vblank_init() ---
commit 4aec980231f556c8a36b5294d67b3d582d49d623
Author: Lyude Paul <lyude@redhat.com>

drm/vblank: Use drm_for_each_crtc() in drm_vblank_init()

This patch updates drm_vblank_init() to use the drm_for_each_crtc() iterator
instead of manually counting the number of CRTCs.

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 17283e8920378..d57405050554f 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -547,19 +547,20 @@ static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
>  int drm_vblank_init(struct drm_device *dev)
>  {
>  	int ret;
> -	unsigned int num_crtcs = dev->mode_config.num_crtc;
> -	unsigned int i;
> +	struct drm_crtc *crtc;
>  
>  	spin_lock_init(&dev->vbl_lock);
>  	spin_lock_init(&dev->vblank_time_lock);
>  
> -	dev->vblank = drmm_kcalloc(dev, num_crtcs, sizeof(*dev->vblank), GFP_KERNEL);
> +	dev->vblank = drmm_kcalloc(dev, dev->mode_config.num_crtc,
> +				   sizeof(*dev->vblank), GFP_KERNEL);
>  	if (!dev->vblank)
>  		return -ENOMEM;
>  
>  	dev->has_hw_vblank = true;
>  
> -	for (i = 0; i < num_crtcs; i++) {
> +	drm_for_each_crtc(crtc, dev) {

[Severity: Critical]
Could this change lead to an uninitialized vblank array and subsequent page
fault for drivers that initialize vblank before creating CRTCs?

For example, in vkms_drv.c:vkms_create(), drm_vblank_init() is called before
vkms_modeset_init() creates the CRTCs:

	ret = drm_vblank_init(&vkms_device->drm);
	if (ret) {
		DRM_ERROR("Failed to vblank\n");
		goto out_devres;
	}

	ret = vkms_modeset_init(vkms_device);

Because the CRTCs are not yet created at this point, dev->mode_config.num_crtc
is 0. The allocation for the vblank array becomes a 0-byte allocation,
returning ZERO_SIZE_PTR (0x10). The drm_for_each_crtc() loop then executes 0
times, and drm_vblank_init() successfully returns 0.

When userspace later calls DRM_IOCTL_WAIT_VBLANK, the kernel looks up the
CRTC using drm_vblank_crtc(dev, pipe). This returns a pointer into the array,
which evaluates to ZERO_SIZE_PTR + pipe * sizeof(...).

Will dereferencing this pointer cause an immediate kernel OOPS?

> +		unsigned int i = crtc->index;
>  		struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, i);
>  
>  		vblank->dev = dev;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 17283e8920378..d57405050554f 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -547,19 +547,20 @@  static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
 int drm_vblank_init(struct drm_device *dev)
 {
 	int ret;
-	unsigned int num_crtcs = dev->mode_config.num_crtc;
-	unsigned int i;
+	struct drm_crtc *crtc;
 
 	spin_lock_init(&dev->vbl_lock);
 	spin_lock_init(&dev->vblank_time_lock);
 
-	dev->vblank = drmm_kcalloc(dev, num_crtcs, sizeof(*dev->vblank), GFP_KERNEL);
+	dev->vblank = drmm_kcalloc(dev, dev->mode_config.num_crtc,
+				   sizeof(*dev->vblank), GFP_KERNEL);
 	if (!dev->vblank)
 		return -ENOMEM;
 
 	dev->has_hw_vblank = true;
 
-	for (i = 0; i < num_crtcs; i++) {
+	drm_for_each_crtc(crtc, dev) {
+		unsigned int i = crtc->index;
 		struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, i);
 
 		vblank->dev = dev;