[PATCH] drm/i915: Always use kref tracking for contexts.

[PATCH] drm/i915: Always use kref tracking for contexts.

[Chris Wilson]

If we always initialize kref for the context, even if we are using fake
contexts for hangstats when there is no hw support, we can forgo the
dance to dereference the ctx->obj and inspect whether we are permitted
to use kref inside i915_gem_context_reference() and _unreference().

My ulterior motive here is to improve the debugging of a use-after-free
of ctx->obj. This patch avoids the dereference here and instead forces
the assertion checks associated with kref.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h         |  6 ++----
 drivers/gpu/drm/i915/i915_gem_context.c | 37 +++++++++++++++++++++------------
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8af8e0dd3943..17a37578053c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2380,14 +2380,12 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
 void i915_gem_context_free(struct kref *ctx_ref);
 static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
 {
-	if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
-		kref_get(&ctx->ref);
+	kref_get(&ctx->ref);
 }
 
 static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
 {
-	if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
-		kref_put(&ctx->ref, i915_gem_context_free);
+	kref_put(&ctx->ref, i915_gem_context_free);
 }
 
 static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e9f888ea67d6..48dc5f8f21bf 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -179,12 +179,9 @@ create_vm_for_ctx(struct drm_device *dev, struct i915_hw_context *ctx)
 }
 
 static struct i915_hw_context *
-__create_hw_context(struct drm_device *dev,
-		  struct drm_i915_file_private *file_priv)
+__ctx_alloc(void)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_hw_context *ctx;
-	int ret;
 
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (ctx == NULL)
@@ -193,6 +190,21 @@ __create_hw_context(struct drm_device *dev,
 	kref_init(&ctx->ref);
 	INIT_LIST_HEAD(&ctx->link);
 
+	return ctx;
+}
+
+static struct i915_hw_context *
+__create_hw_context(struct drm_device *dev,
+		  struct drm_i915_file_private *file_priv)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_hw_context *ctx;
+	int ret;
+
+	ctx = __ctx_alloc();
+	if (IS_ERR(ctx))
+		return ctx;
+
 	ctx->obj = i915_gem_object_create_stolen(dev, dev_priv->hw_context_size);
 	if (ctx->obj == NULL)
 		ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
@@ -492,11 +504,9 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 
 	if (!HAS_HW_CONTEXTS(dev)) {
 		/* Cheat for hang stats */
-		file_priv->private_default_ctx =
-			kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
-
-		if (file_priv->private_default_ctx == NULL)
-			return -ENOMEM;
+		file_priv->private_default_ctx = __ctx_alloc();
+		if (IS_ERR(file_priv->private_default_ctx))
+			return PTR_ERR(file_priv->private_default_ctx);
 
 		file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
 		return 0;
@@ -521,14 +531,15 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 
-	if (!HAS_HW_CONTEXTS(dev)) {
-		kfree(file_priv->private_default_ctx);
+	if (IS_ERR_OR_NULL(file_priv->private_default_ctx))
 		return;
+
+	if (HAS_HW_CONTEXTS(dev)) {
+		idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
+		idr_destroy(&file_priv->context_idr);
 	}
 
-	idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
 	i915_gem_context_unreference(file_priv->private_default_ctx);
-	idr_destroy(&file_priv->context_idr);
 }
 
 struct i915_hw_context *
-- 
1.9.0

Re: [PATCH] drm/i915: Always use kref tracking for contexts.

[Ben Widawsky]

On Fri, Feb 28, 2014 at 08:06:50PM +0000, Chris Wilson wrote:
> If we always initialize kref for the context, even if we are using fake
> contexts for hangstats when there is no hw support, we can forgo the
> dance to dereference the ctx->obj and inspect whether we are permitted
> to use kref inside i915_gem_context_reference() and _unreference().
> 
> My ulterior motive here is to improve the debugging of a use-after-free
> of ctx->obj. This patch avoids the dereference here and instead forces
> the assertion checks associated with kref.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  6 ++----
>  drivers/gpu/drm/i915/i915_gem_context.c | 37 +++++++++++++++++++++------------
>  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8af8e0dd3943..17a37578053c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2380,14 +2380,12 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
>  void i915_gem_context_free(struct kref *ctx_ref);
>  static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
>  {
> -	if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> -		kref_get(&ctx->ref);
> +	kref_get(&ctx->ref);
>  }
>  
>  static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
>  {
> -	if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> -		kref_put(&ctx->ref, i915_gem_context_free);
> +	kref_put(&ctx->ref, i915_gem_context_free);
>  }
>  
>  static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e9f888ea67d6..48dc5f8f21bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -179,12 +179,9 @@ create_vm_for_ctx(struct drm_device *dev, struct i915_hw_context *ctx)
>  }
>  
>  static struct i915_hw_context *
> -__create_hw_context(struct drm_device *dev,
> -		  struct drm_i915_file_private *file_priv)
> +__ctx_alloc(void)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct i915_hw_context *ctx;
> -	int ret;
>  
>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>  	if (ctx == NULL)
> @@ -193,6 +190,21 @@ __create_hw_context(struct drm_device *dev,
>  	kref_init(&ctx->ref);
>  	INIT_LIST_HEAD(&ctx->link);
>  
> +	return ctx;
> +}
> +
> +static struct i915_hw_context *
> +__create_hw_context(struct drm_device *dev,
> +		  struct drm_i915_file_private *file_priv)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_hw_context *ctx;
> +	int ret;
> +
> +	ctx = __ctx_alloc();
> +	if (IS_ERR(ctx))
> +		return ctx;
> +

>  	ctx->obj = i915_gem_object_create_stolen(dev, dev_priv->hw_context_size);
		  How this working for you ^ ?


>  	if (ctx->obj == NULL)
>  		ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> @@ -492,11 +504,9 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
>  
>  	if (!HAS_HW_CONTEXTS(dev)) {
>  		/* Cheat for hang stats */
> -		file_priv->private_default_ctx =
> -			kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
> -
> -		if (file_priv->private_default_ctx == NULL)
> -			return -ENOMEM;
> +		file_priv->private_default_ctx = __ctx_alloc();
> +		if (IS_ERR(file_priv->private_default_ctx))
> +			return PTR_ERR(file_priv->private_default_ctx);
>  
>  		file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
>  		return 0;
> @@ -521,14 +531,15 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  
> -	if (!HAS_HW_CONTEXTS(dev)) {
> -		kfree(file_priv->private_default_ctx);
> +	if (IS_ERR_OR_NULL(file_priv->private_default_ctx))
>  		return;
> +
> +	if (HAS_HW_CONTEXTS(dev)) {
> +		idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> +		idr_destroy(&file_priv->context_idr);
>  	}
>  
> -	idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
>  	i915_gem_context_unreference(file_priv->private_default_ctx);
> -	idr_destroy(&file_priv->context_idr);
>  }

I don't see too much harm in just initializing the idr regardless to
keep the close path simpler. Then stick a WARN in context_idr_cleanup if
it actually does anything, fake_context_idr_cleanup();

I have some recollection of hitting a really tricky thing while
developing PPGTT where the order of the unref in the above sequence
really mattered. Looking again though, I don't see anything familiar
- but my suggestion would put me completely at ease.

In either case:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

>  
>  struct i915_hw_context *

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Always use kref tracking for contexts.

[Chris Wilson]

On Sun, Mar 02, 2014 at 03:58:09PM -0800, Ben Widawsky wrote:
> On Fri, Feb 28, 2014 at 08:06:50PM +0000, Chris Wilson wrote:
> >  	ctx->obj = i915_gem_object_create_stolen(dev, dev_priv->hw_context_size);
> 		  How this working for you ^ ?

Fine. It helps detect cases where we try to load uninitialised contexts,
but other than that I have not noticed any difference.

> > +	if (HAS_HW_CONTEXTS(dev)) {
> > +		idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> > +		idr_destroy(&file_priv->context_idr);
> >  	}
> >  
> > -	idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> >  	i915_gem_context_unreference(file_priv->private_default_ctx);
> > -	idr_destroy(&file_priv->context_idr);
> >  }
> 
> I don't see too much harm in just initializing the idr regardless to
> keep the close path simpler. Then stick a WARN in context_idr_cleanup if
> it actually does anything, fake_context_idr_cleanup();

Agreed I didn't see any harm in making that change as well. I was just
trying to keep the set of changes small, and targetted towards removing
that ctx->obj dereference.
 
> I have some recollection of hitting a really tricky thing while
> developing PPGTT where the order of the unref in the above sequence
> really mattered. Looking again though, I don't see anything familiar
> - but my suggestion would put me completely at ease.

Sure, but I think you've squashed that bug already. :)
 
> In either case:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Ok, let's go with a second patch to converge the fake context with the
hw context later.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Always use kref tracking for contexts.

[Daniel Vetter]

On Mon, Mar 03, 2014 at 07:19:19AM +0000, Chris Wilson wrote:
> On Sun, Mar 02, 2014 at 03:58:09PM -0800, Ben Widawsky wrote:
> > On Fri, Feb 28, 2014 at 08:06:50PM +0000, Chris Wilson wrote:
> > >  	ctx->obj = i915_gem_object_create_stolen(dev, dev_priv->hw_context_size);
> > 		  How this working for you ^ ?
> 
> Fine. It helps detect cases where we try to load uninitialised contexts,
> but other than that I have not noticed any difference.

Separate patch pls with a note that we have a higher chance of blowing up
on random stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Always use kref tracking for contexts.

[Chris Wilson]

On Wed, Mar 05, 2014 at 06:43:03PM +0100, Daniel Vetter wrote:
> On Mon, Mar 03, 2014 at 07:19:19AM +0000, Chris Wilson wrote:
> > On Sun, Mar 02, 2014 at 03:58:09PM -0800, Ben Widawsky wrote:
> > > On Fri, Feb 28, 2014 at 08:06:50PM +0000, Chris Wilson wrote:
> > > >  	ctx->obj = i915_gem_object_create_stolen(dev, dev_priv->hw_context_size);
> > > 		  How this working for you ^ ?
> > 
> > Fine. It helps detect cases where we try to load uninitialised contexts,
> > but other than that I have not noticed any difference.
> 
> Separate patch pls with a note that we have a higher chance of blowing up
> on random stuff.

It is a separate patch. It's in the contextual diff of the patch that
Ben spotted a change I have been using for ages.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre