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

[Daniel Vetter]

On Fri, Apr 15, 2016 at 12:55:08PM +0100, 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.
> v3: Needed to update the return value check after changing from using
> the stale error pointer to NULL.
> 
> 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>

I added a note about the intended use-case of this and merged it do
drm-misc.
-Daniel

> ---
>  drivers/gpu/drm/drm_gem.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index da0c5320789f..e97b7a99807b 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -279,7 +279,6 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>  int
>  drm_gem_handle_delete(struct drm_file *filp, u32 handle)
>  {
> -	struct drm_device *dev;
>  	struct drm_gem_object *obj;
>  
>  	/* This is gross. The idr system doesn't let us try a delete and
> @@ -294,18 +293,19 @@ 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) {
> -		spin_unlock(&filp->table_lock);
> +	obj = idr_replace(&filp->object_idr, NULL, handle);
> +	spin_unlock(&filp->table_lock);
> +	if (IS_ERR_OR_NULL(obj))
>  		return -EINVAL;
> -	}
> -	dev = obj->dev;
>  
> -	/* Release reference and decrement refcount. */
> +	/* Release driver's reference and decrement refcount. */
> +	drm_gem_object_release_handle(handle, obj, filp);
> +
> +	/* And finally make the handle available for future allocations. */
> +	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.8.0.rc3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch