[PATCH] drm: Release driver tracking before making the object available again

[PATCH] drm: Release driver tracking before making the object available again

[Chris Wilson]

This is the same bug as we fixed in commit f6cd7daecff5 ("drm: Release
driver references to handle before making it available again"), but now
the exposure is via the PRIME lookup tables. If we remove the
object/handle from the PRIME lut, then a new request for the same
object/fd will generate a new handle, thus for a short window that
object is known to userspace by two different handles. Fix this by
releasing the driver tracking before PRIME.

Fixes: 0ff926c7d4f0 ("drm/prime: add exported buffers to current fprivs
imported buffer list (v2)")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
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>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_gem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index ad4e9cfe48a2..88c6d78ee2d5 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -255,13 +255,13 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
 	struct drm_gem_object *obj = ptr;
 	struct drm_device *dev = obj->dev;
 
+	if (dev->driver->gem_close_object)
+		dev->driver->gem_close_object(obj, file_priv);
+
 	if (drm_core_check_feature(dev, DRIVER_PRIME))
 		drm_gem_remove_prime_handles(obj, file_priv);
 	drm_vma_node_revoke(&obj->vma_node, file_priv);
 
-	if (dev->driver->gem_close_object)
-		dev->driver->gem_close_object(obj, file_priv);
-
 	drm_gem_object_handle_put_unlocked(obj);
 
 	return 0;
-- 
2.14.1

Re: [PATCH] drm: Release driver tracking before making the object available again

[Daniel Vetter]

On Sat, Aug 19, 2017 at 01:05:58PM +0100, Chris Wilson wrote:
> This is the same bug as we fixed in commit f6cd7daecff5 ("drm: Release
> driver references to handle before making it available again"), but now
> the exposure is via the PRIME lookup tables. If we remove the
> object/handle from the PRIME lut, then a new request for the same
> object/fd will generate a new handle, thus for a short window that
> object is known to userspace by two different handles. Fix this by
> releasing the driver tracking before PRIME.
> 
> Fixes: 0ff926c7d4f0 ("drm/prime: add exported buffers to current fprivs
> imported buffer list (v2)")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 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>
> Cc: stable@vger.kernel.org

Do we have an evil igt for this? I guess since the old one didn't have
one, this new race is also hard to reproduce ...

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_gem.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ad4e9cfe48a2..88c6d78ee2d5 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -255,13 +255,13 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>  	struct drm_gem_object *obj = ptr;
>  	struct drm_device *dev = obj->dev;
>  
> +	if (dev->driver->gem_close_object)
> +		dev->driver->gem_close_object(obj, file_priv);
> +
>  	if (drm_core_check_feature(dev, DRIVER_PRIME))
>  		drm_gem_remove_prime_handles(obj, file_priv);
>  	drm_vma_node_revoke(&obj->vma_node, file_priv);
>  
> -	if (dev->driver->gem_close_object)
> -		dev->driver->gem_close_object(obj, file_priv);
> -
>  	drm_gem_object_handle_put_unlocked(obj);
>  
>  	return 0;
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Release driver tracking before making the object available again

[Chris Wilson]

Quoting Daniel Vetter (2017-08-21 17:16:24)
> On Sat, Aug 19, 2017 at 01:05:58PM +0100, Chris Wilson wrote:
> > This is the same bug as we fixed in commit f6cd7daecff5 ("drm: Release
> > driver references to handle before making it available again"), but now
> > the exposure is via the PRIME lookup tables. If we remove the
> > object/handle from the PRIME lut, then a new request for the same
> > object/fd will generate a new handle, thus for a short window that
> > object is known to userspace by two different handles. Fix this by
> > releasing the driver tracking before PRIME.
> > 
> > Fixes: 0ff926c7d4f0 ("drm/prime: add exported buffers to current fprivs
> > imported buffer list (v2)")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 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>
> > Cc: stable@vger.kernel.org
> 
> Do we have an evil igt for this? I guess since the old one didn't have
> one, this new race is also hard to reproduce ...

The old one we did hit in igt (gem_concurrent_blit), but only by virtue
of it running for long enough to spot the race (ending with two handles
to the same object in an execbuf call).

This one requires us racing dma-buf import/close vs execbuf on the same
handles. It's the type of race the gem_close_race is looking for (except
that it doesn't cover dmabuf yet), but we are reliant on having a means
to detect the race. At the moment, we would detect it if you ended up
with two handles to the same object within the execbuf (which is
plausible as you can currently create that second handle before we mark
the first as closed, but the race will require some unfair queueing on
struct_mutex), or if we end up with two handles to the vma on close.

Hmm, the one way to make the race easier to hit is to add a sleep to
i915_gem_close_object before we take the struct_mutex.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Release driver tracking before making the object available again

[Joonas Lahtinen]

+ Sean

On Mon, 2017-08-21 at 18:16 +0200, Daniel Vetter wrote:
> On Sat, Aug 19, 2017 at 01:05:58PM +0100, Chris Wilson wrote:
> > This is the same bug as we fixed in commit f6cd7daecff5 ("drm: Release
> > driver references to handle before making it available again"), but now
> > the exposure is via the PRIME lookup tables. If we remove the
> > object/handle from the PRIME lut, then a new request for the same
> > object/fd will generate a new handle, thus for a short window that
> > object is known to userspace by two different handles. Fix this by
> > releasing the driver tracking before PRIME.
> > 
> > Fixes: 0ff926c7d4f0 ("drm/prime: add exported buffers to current fprivs
> > imported buffer list (v2)")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 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>
> > Cc: stable@vger.kernel.org
> 
> Do we have an evil igt for this? I guess since the old one didn't have
> one, this new race is also hard to reproduce ...
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed this to drm-misc-fixes (and drm-misc-next for I am a monkey with
a keyboard), thanks for the patch and review.

Sean, you can blame it on me when/if there is trouble caused by the
patch being in both branches. Hopefully next merge will cause less
headache.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel