diff mbox

drm: Fix race when checking for fb in the generic kms obj lookup

Message ID 1406196765-2428-1-git-send-email-daniel.vetter@ffwll.ch
State Accepted
Headers show

Commit Message

Daniel Vetter July 24, 2014, 10:12 a.m. UTC
In my review of

commit 98f75de40e9d83c3a90d294b8fd25fa2874212a9
Author: Rob Clark <robdclark@gmail.com>
Date:   Fri May 30 11:37:03 2014 -0400

    drm: add object property typ

I asked for a check to make sure that we never leak an fb from the
generic mode object lookup since those have completely different
lifetime rules. Rob added it, but outside of the idr mutex, which
means that our dereference of obj->type can already chase free'd
memory.

Somehow I didn't spot this, so fix this asap.

v2: Simplify the conditionals as suggested by Chris.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Rob Clark July 25, 2014, 10:16 a.m. UTC | #1
On Thu, Jul 24, 2014 at 6:12 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> In my review of
>
> commit 98f75de40e9d83c3a90d294b8fd25fa2874212a9
> Author: Rob Clark <robdclark@gmail.com>
> Date:   Fri May 30 11:37:03 2014 -0400
>
>     drm: add object property typ
>
> I asked for a check to make sure that we never leak an fb from the
> generic mode object lookup since those have completely different
> lifetime rules. Rob added it, but outside of the idr mutex, which
> means that our dereference of obj->type can already chase free'd
> memory.
>
> Somehow I didn't spot this, so fix this asap.
>
> v2: Simplify the conditionals as suggested by Chris.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_crtc.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f0a777747907..d87df8836aa5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -426,8 +426,12 @@ static struct drm_mode_object *_object_find(struct drm_device *dev,
>
>         mutex_lock(&dev->mode_config.idr_mutex);
>         obj = idr_find(&dev->mode_config.crtc_idr, id);
> -       if (!obj || (type != DRM_MODE_OBJECT_ANY && obj->type != type) ||
> -           (obj->id != id))
> +       if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type)
> +               obj = NULL;
> +       if (obj && obj->id != id)
> +               obj = NULL;
> +       /* don't leak out unref'd fb's */
> +       if (obj && (obj->type == DRM_MODE_OBJECT_FB))
>                 obj = NULL;
>         mutex_unlock(&dev->mode_config.idr_mutex);
>
> @@ -454,9 +458,6 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>          * function.*/
>         WARN_ON(type == DRM_MODE_OBJECT_FB);
>         obj = _object_find(dev, id, type);
> -       /* don't leak out unref'd fb's */
> -       if (obj && (obj->type == DRM_MODE_OBJECT_FB))
> -               obj = NULL;
>         return obj;
>  }
>  EXPORT_SYMBOL(drm_mode_object_find);
> --
> 2.0.1
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f0a777747907..d87df8836aa5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -426,8 +426,12 @@  static struct drm_mode_object *_object_find(struct drm_device *dev,
 
 	mutex_lock(&dev->mode_config.idr_mutex);
 	obj = idr_find(&dev->mode_config.crtc_idr, id);
-	if (!obj || (type != DRM_MODE_OBJECT_ANY && obj->type != type) ||
-	    (obj->id != id))
+	if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type)
+		obj = NULL;
+	if (obj && obj->id != id)
+		obj = NULL;
+	/* don't leak out unref'd fb's */
+	if (obj && (obj->type == DRM_MODE_OBJECT_FB))
 		obj = NULL;
 	mutex_unlock(&dev->mode_config.idr_mutex);
 
@@ -454,9 +458,6 @@  struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 	 * function.*/
 	WARN_ON(type == DRM_MODE_OBJECT_FB);
 	obj = _object_find(dev, id, type);
-	/* don't leak out unref'd fb's */
-	if (obj && (obj->type == DRM_MODE_OBJECT_FB))
-		obj = NULL;
 	return obj;
 }
 EXPORT_SYMBOL(drm_mode_object_find);