On Fri, Apr 05, 2019 at 08:41:16AM +0100, Chris Wilson wrote: > Quoting Janusz Krzysztofik (2019-04-05 08:26:57) > > From: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxx> > > > > The driver does not currently support unbinding from a device which is > > in use. Since open file descriptors may still be pointing into kernel > > memory where the device structures used to be, entirely correct kernel > > panics protect the driver from being unbound as we should not be > > unbinding it before those dangling pointers have been made safe. > > > > According to the documentation found inside drivers/gpu/drm/drm_drv.c, > > drm_dev_unplug() should be used instead of drm_dev_unregister() in > > order to make a device inaccessible to users as soon as it is unpluged. > > Follow that advice to make those possibly dangling pointers safe, > > protected by DRM layer from a user who is otherwise left pointing into > > possibly reused kernel memory after the driver has been unbound from > > the device. > > > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 9df65d386d11..66163378c481 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1596,7 +1596,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > > i915_pmu_unregister(dev_priv); > > > > i915_teardown_sysfs(dev_priv); > > - drm_dev_unregister(&dev_priv->drm); > > + drm_dev_unplug(&dev_priv->drm); > > I think we may have our onion inverted here. We want to stop the users > as the first step, then start removing the entries. (That will also > nicely invert the order from register, which is what we typically > expect). > > After calling i915_driver_unregister(); call i915_gem_set_wedged() to > immediately (give or take external fences) cancel inflight operations. I think we still need the above patch, since drm_dev_unplug == drm_dev_unregister + "make sure userspace can't get at us anymore". We could/should probably drop drm_dev_unplug and move that additional code to drm_dev_unregister, but there's some minutea in how we refcount the drm_device between the two. So not quite as clean a job. There's also drm_put_dev (not to be mistaken with drm_dev_put), for added confusion. I think ideally we'd unify all three of drm_dev_unregister, drm_dev_unplug and drm_put_dev to one, deprecating all the others. But that's work :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
