[PATCH] drm: Fixup racy refcounting in plane_force_disable

[PATCH] drm: Fixup racy refcounting in plane_force_disable

[Daniel Vetter]

Originally it was impossible to be dropping the last refcount in this
function since there was always one around still from the idr. But in

commit 83f45fc360c8e16a330474860ebda872d1384c8c
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Aug 6 09:10:18 2014 +0200

    drm: Don't grab an fb reference for the idr

we've switched to weak references, broke that assumption but forgot to
fix it up.

Since we still force-disable planes it's only possible to hit this
when racing multiple rmfb with fbdev restoring or similar evil things.
As long as userspace is nice it's impossible to hit the BUG_ON.

But the BUG_ON would most likely be hit from fbdev code, which usually
invovles the console_lock besides all modeset locks. So very likely
we'd never get the bug reports if this was hit in the wild, hence
better be safe than sorry and backport.

Spotted by Matt Roper while reviewing other patches.

Cc: stable@vger.kernel.org
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index cc548ecd3634..897f51beaadd 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -524,17 +524,6 @@ void drm_framebuffer_reference(struct drm_framebuffer *fb)
 }
 EXPORT_SYMBOL(drm_framebuffer_reference);
 
-static void drm_framebuffer_free_bug(struct kref *kref)
-{
-	BUG();
-}
-
-static void __drm_framebuffer_unreference(struct drm_framebuffer *fb)
-{
-	DRM_DEBUG("%p: FB ID: %d (%d)\n", fb, fb->base.id, atomic_read(&fb->refcount.refcount));
-	kref_put(&fb->refcount, drm_framebuffer_free_bug);
-}
-
 /**
  * drm_framebuffer_unregister_private - unregister a private fb from the lookup idr
  * @fb: fb to unregister
@@ -1319,7 +1308,7 @@ void drm_plane_force_disable(struct drm_plane *plane)
 		return;
 	}
 	/* disconnect the plane from the fb and crtc: */
-	__drm_framebuffer_unreference(plane->old_fb);
+	drm_framebuffer_unreference(plane->old_fb);
 	plane->old_fb = NULL;
 	plane->fb = NULL;
 	plane->crtc = NULL;
-- 
2.1.4

Re: [PATCH] drm: Fixup racy refcounting in plane_force_disable

[Matt Roper]

On Fri, Feb 27, 2015 at 01:03:37PM +0100, Daniel Vetter wrote:
> Originally it was impossible to be dropping the last refcount in this
> function since there was always one around still from the idr. But in
> 
> commit 83f45fc360c8e16a330474860ebda872d1384c8c
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Aug 6 09:10:18 2014 +0200
> 
>     drm: Don't grab an fb reference for the idr
> 
> we've switched to weak references, broke that assumption but forgot to
> fix it up.
> 
> Since we still force-disable planes it's only possible to hit this
> when racing multiple rmfb with fbdev restoring or similar evil things.
> As long as userspace is nice it's impossible to hit the BUG_ON.
> 
> But the BUG_ON would most likely be hit from fbdev code, which usually
> invovles the console_lock besides all modeset locks. So very likely
> we'd never get the bug reports if this was hit in the wild, hence
> better be safe than sorry and backport.
> 
> Spotted by Matt Roper while reviewing other patches.
> 
> Cc: stable@vger.kernel.org
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/drm_crtc.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index cc548ecd3634..897f51beaadd 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -524,17 +524,6 @@ void drm_framebuffer_reference(struct drm_framebuffer *fb)
>  }
>  EXPORT_SYMBOL(drm_framebuffer_reference);
>  
> -static void drm_framebuffer_free_bug(struct kref *kref)
> -{
> -	BUG();
> -}
> -
> -static void __drm_framebuffer_unreference(struct drm_framebuffer *fb)
> -{
> -	DRM_DEBUG("%p: FB ID: %d (%d)\n", fb, fb->base.id, atomic_read(&fb->refcount.refcount));
> -	kref_put(&fb->refcount, drm_framebuffer_free_bug);
> -}
> -
>  /**
>   * drm_framebuffer_unregister_private - unregister a private fb from the lookup idr
>   * @fb: fb to unregister
> @@ -1319,7 +1308,7 @@ void drm_plane_force_disable(struct drm_plane *plane)
>  		return;
>  	}
>  	/* disconnect the plane from the fb and crtc: */
> -	__drm_framebuffer_unreference(plane->old_fb);
> +	drm_framebuffer_unreference(plane->old_fb);
>  	plane->old_fb = NULL;
>  	plane->fb = NULL;
>  	plane->crtc = NULL;
> -- 
> 2.1.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795