| Message ID | 1393618010-24070-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 *
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
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
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
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 *
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(-)