| Message ID | 20170819120558.6465-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
+ 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
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;
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(-)