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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

Otherwise makes sense so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	/* Release reference and decrement refcount. */
> +	drm_gem_object_release_handle(handle, obj, filp);
> +
> +	spin_lock(&filp->table_lock);
>  	idr_remove(&filp->object_idr, handle);
>  	spin_unlock(&filp->table_lock);
>  
> -	drm_gem_object_release_handle(handle, obj, filp);
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_handle_delete);
> -- 
> 2.7.0.rc3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel