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

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

[Chris Wilson]

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. It is then possible for a lookup on that handle to
return an error object and so all callers of idr_find(file->object_idr)
need to check against IS_ERR_OR_NULL() instead.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: dri-devel@lists.freedesktop.org
Cc: David Airlie <airlied@linux.ie>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@arm.linux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_gem.c                    | 11 +++++++----
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c   |  2 +-
 drivers/gpu/drm/msm/msm_gem_submit.c         |  2 +-
 drivers/gpu/drm/vc4/vc4_gem.c                |  2 +-
 5 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2e8c77e71e1f..c56a0b49d829 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, ERR_PTR(-ENOENT), handle);
+	if (IS_ERR(obj)) {
 		spin_unlock(&filp->table_lock);
 		return -EINVAL;
 	}
 	dev = obj->dev;
+	spin_unlock(&filp->table_lock);
 
 	/* 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);
@@ -597,7 +600,7 @@ drm_gem_object_lookup(struct drm_device *dev, struct drm_file *filp,
 
 	/* Check if we currently have a reference on the object */
 	obj = idr_find(&filp->object_idr, handle);
-	if (obj == NULL) {
+	if (IS_ERR_OR_NULL(obj)) {
 		spin_unlock(&filp->table_lock);
 		return NULL;
 	}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 1aba01a999df..77b549cb1301 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -78,7 +78,7 @@ static int submit_lookup_objects(struct etnaviv_gem_submit *submit,
 		 * all under single table_lock just hit object_idr directly:
 		 */
 		obj = idr_find(&file->object_idr, bo->handle);
-		if (!obj) {
+		if (IS_ERR_OR_NULL(obj)) {
 			DRM_ERROR("invalid handle %u at index %u\n",
 				  bo->handle, i);
 			ret = -EINVAL;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index dccb517361b3..500bcff9ad48 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -106,7 +106,7 @@ eb_lookup_vmas(struct eb_vmas *eb,
 	 * or create the VMA without using GFP_ATOMIC */
 	for (i = 0; i < args->buffer_count; i++) {
 		obj = to_intel_bo(idr_find(&file->object_idr, exec[i].handle));
-		if (obj == NULL) {
+		if (IS_ERR_OR_NULL(obj)) {
 			spin_unlock(&file->table_lock);
 			DRM_DEBUG("Invalid object handle %d at index %d\n",
 				   exec[i].handle, i);
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 6d7cd3fe21e7..c6d7abd53d20 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -90,7 +90,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 		 * all under single table_lock just hit object_idr directly:
 		 */
 		obj = idr_find(&file->object_idr, submit_bo.handle);
-		if (!obj) {
+		if (IS_ERR_OR_NULL(obj)) {
 			DRM_ERROR("invalid handle %u at index %u\n", submit_bo.handle, i);
 			ret = -EINVAL;
 			goto out_unlock;
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 48ce30a6f4b5..9138702a1c67 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -494,7 +494,7 @@ vc4_cl_lookup_bos(struct drm_device *dev,
 	for (i = 0; i < exec->bo_count; i++) {
 		struct drm_gem_object *bo = idr_find(&file_priv->object_idr,
 						     handles[i]);
-		if (!bo) {
+		if (IS_ERR_OR_NULL(bo)) {
 			DRM_ERROR("Failed to look up GEM BO %d: %d\n",
 				  i, handles[i]);
 			ret = -EINVAL;
-- 
2.7.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

On Fri, Jan 08, 2016 at 08:25:07PM +0000, Chris Wilson wrote:
> 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. It is then possible for a lookup on that handle to
> return an error object and so all callers of idr_find(file->object_idr)
> need to check against IS_ERR_OR_NULL() instead.
> 
> -	obj = idr_find(&filp->object_idr, handle);
> -	if (obj == NULL) {
> +	obj = idr_replace(&filp->object_idr, ERR_PTR(-ENOENT), handle);
> +	if (IS_ERR(obj)) {

Setting ERR_PTR(-ENOENT) is superfluous. We can just set the entry to
NULL as idr_alloc() uses a bitmap to find the new handle. This avoids
having to change the callers elsewhere, as idr_find() will then return
NULL for the stale handle and the invalid handle.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

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);
 
 	/* 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on ff88655b3a5467bbc3be8c67d3e05ebf182557d3 drm-intel-nightly: 2016y-01m-11d-07h-30m-16s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (bdw-nuci7)
                dmesg-warn -> PASS       (bdw-ultra)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (byt-nuc)

bdw-nuci7        total:138  pass:128  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:138  pass:132  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:141  pass:114  dwarn:3   dfail:0   fail:0   skip:24 
byt-nuc          total:141  pass:119  dwarn:7   dfail:0   fail:0   skip:15 
hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:141  pass:137  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:141  pass:101  dwarn:3   dfail:0   fail:0   skip:37 
ivb-t430s        total:135  pass:122  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:141  pass:132  dwarn:1   dfail:0   fail:0   skip:8  
skl-i7k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1119/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on ff88655b3a5467bbc3be8c67d3e05ebf182557d3 drm-intel-nightly: 2016y-01m-11d-07h-30m-16s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (bdw-nuci7)
                dmesg-warn -> PASS       (bdw-ultra)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (byt-nuc)

bdw-nuci7        total:138  pass:128  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:138  pass:132  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:141  pass:114  dwarn:3   dfail:0   fail:0   skip:24 
byt-nuc          total:141  pass:119  dwarn:7   dfail:0   fail:0   skip:15 
hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:141  pass:137  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:141  pass:101  dwarn:3   dfail:0   fail:0   skip:37 
ivb-t430s        total:135  pass:122  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:141  pass:132  dwarn:1   dfail:0   fail:0   skip:8  
skl-i7k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1119/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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.

Hmm. What's the problem with another object starting to reuse the same
handle while we're still deleting the old one? So far I didn't spot
anything in the code that would go boom if there's another object
already around with the same handle.

> 
> 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);
>  
>  	/* 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

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

[Chris Wilson]

On Mon, Jan 11, 2016 at 07:51:03PM +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.
> 
> Hmm. What's the problem with another object starting to reuse the same
> handle while we're still deleting the old one? So far I didn't spot
> anything in the code that would go boom if there's another object
> already around with the same handle.

Imagine a driver storing a hashtable to contract the handle->object->vma
lookup into just handle->vma, like the old idea of replacing the
object_idr with an ida plus hashtable of objects. (This saves the double
step lookup that caused a regression with the ppgtt work, and the linear
walk of object->vma_list which is a major slowdown in various full-pggtt
OpenGL tests). In such a scheme, where the driver has a parallel lut to
the core, the driver needs to be notified before the handle is then
accessible again to userspace. Or in any other scenario where the driver
is using the handle, as would be implied by having the open/close
callbacks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Mon, Jan 11, 2016 at 08:44:03PM +0000, Chris Wilson wrote:
> On Mon, Jan 11, 2016 at 07:51:03PM +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.
> > 
> > Hmm. What's the problem with another object starting to reuse the same
> > handle while we're still deleting the old one? So far I didn't spot
> > anything in the code that would go boom if there's another object
> > already around with the same handle.
> 
> Imagine a driver storing a hashtable to contract the handle->object->vma
> lookup into just handle->vma, like the old idea of replacing the
> object_idr with an ida plus hashtable of objects. (This saves the double
> step lookup that caused a regression with the ppgtt work, and the linear
> walk of object->vma_list which is a major slowdown in various full-pggtt
> OpenGL tests). In such a scheme, where the driver has a parallel lut to
> the core, the driver needs to be notified before the handle is then
> accessible again to userspace. Or in any other scenario where the driver
> is using the handle, as would be implied by having the open/close
> callbacks.

I see. Yeah, that makes sense. I was just a bit confused since I
couldn't find any real problem in the tree currently.

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

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

[Ville Syrjälä]

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

Re: [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@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.

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel