[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

✓ Fi.CI.BAT: success for drm: Release driver tracking before making the object available again

[Patchwork]

== Series Details ==

Series: drm: Release driver tracking before making the object available again
URL   : https://patchwork.freedesktop.org/series/29039/
State : success

== Summary ==

Series 29039v1 drm: Release driver tracking before making the object available again
https://patchwork.freedesktop.org/api/1.0/series/29039/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test drv_module_reload:
        Subgroup basic-no-display:
                dmesg-warn -> PASS       (fi-kbl-7260u)
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (fi-kbl-7260u) fdo#102295

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#102295 https://bugs.freedesktop.org/show_bug.cgi?id=102295

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:456s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:359s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:558s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:519s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:522s
fi-byt-n2820     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:522s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:612s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:443s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:424s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:420s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:507s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:477s
fi-kbl-7260u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:495s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:481s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:598s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:595s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:527s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:468s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:486s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:445s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:473s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:548s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:405s
fi-bdw-gvtdvm failed to connect after reboot

5b151fd0dafb8130ce4b809e3795146a5ef6bc2e drm-tip: 2017y-08m-18d-22h-38m-58s UTC integration manifest
fde442024d1e drm: Release driver tracking before making the object available again

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5447/
_______________________________________________
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

[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

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

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