[PATCH v2] drm: Release driver references to handle before making it available again

[Chris Wilson]

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 at chris-wilson.co.uk>
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: David Airlie <airlied at linux.ie>
> > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: Rob Clark <robdclark at gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Thierry Reding <treding at nvidia.com>
> > ---
> >  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