On Tue, Jan 12, 2016 at 12:19:12PM +0200, Ville Syrjälä wrote: > On Fri, Jan 08, 2016 at 11:27:05PM +0000, Chris Wilson wrote: > > When userspace closes a handle, we remove it from the file->object_idr > > and then tell the driver to drop its references to that file/handle. > > However, as the file/handle is already available again for reuse, it may > > be reallocated back to userspace and active on a new object before the > > driver has had a chance to drop the old file/handle references. > > > > Whilst calling back into the driver, we have to drop the > > file->table_lock spinlock and so to prevent reusing the closed handle we > > mark that handle as stale in the idr, perform the callback and then > > remove the handle. We set the stale handle to point to the NULL object, > > then any idr_find() whilst the driver is removing the handle will return > > NULL, just as if the handle is already removed from idr. > > > > v2: Use NULL rather than an ERR_PTR to avoid having to adjust callers. > > idr_alloc() tracks existing handles using an internal bitmap, so we are > > free to use the NULL object as our stale identifier. > > > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > Cc: David Airlie <airlied@xxxxxxxx> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > Cc: Rob Clark <robdclark@xxxxxxxxx> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Cc: Thierry Reding <treding@xxxxxxxxxx> > > --- > > drivers/gpu/drm/drm_gem.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 2e8c77e71e1f..d1909d1a1eb4 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -294,18 +294,21 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle) > > spin_lock(&filp->table_lock); > > > > /* Check if we currently have a reference on the object */ > > - obj = idr_find(&filp->object_idr, handle); > > - if (obj == NULL) { > > + obj = idr_replace(&filp->object_idr, NULL, handle); > > + if (IS_ERR(obj)) { > > spin_unlock(&filp->table_lock); > > return -EINVAL; > > } > > dev = obj->dev; > > + spin_unlock(&filp->table_lock); > > Could shrink the spinlocked section to be just the idr_replace() > call I suppose, and thus avoid the spin_unlock() in the error path. Indeed, missed that. I also missed in v2 that the IS_ERR(obj) test needed to become IS_ERR_OR_NULL(obj) to catch the concurrent deletion. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel