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

[Ben Widawsky <ben@bwidawsk.net>]

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