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 <ch...@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 <b...@bwidawsk.net>
>
> struct i915_hw_context *
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
