drm/i915: Use drm_dev_unplug()

Commit Message

Janusz Krzysztofik April 5, 2019, 7:26 a.m. UTC

From: Janusz Krzysztofik <[e-mail]>

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 <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Chris Wilson April 5, 2019, 7:41 a.m. UTC | #1

Quoting Janusz Krzysztofik (2019-04-05 08:26:57)
> From: Janusz Krzysztofik <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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.
-Chris

Janusz Krzysztofik April 5, 2019, 8:11 a.m. UTC | #2

On Fri, 2019-04-05 at 08:41 +0100, Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-04-05 08:26:57)
> > From: Janusz Krzysztofik <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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.

OK, thanks.  Do you prefer them squashed or as serparate patches?

Thanks,
Janusz

> -Chris
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Chris Wilson April 5, 2019, 8:24 a.m. UTC | #3

Quoting Janusz Krzysztofik (2019-04-05 09:11:54)
> On Fri, 2019-04-05 at 08:41 +0100, Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-04-05 08:26:57)
> > > From: Janusz Krzysztofik <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > ---
> > >  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.
> 
> OK, thanks.  Do you prefer them squashed or as serparate patches?

Quite happy to do the s/unregister/unplug/ and move in one go. Have a
pre-emptive
Reviewed-by: Chris Wilson <[e-mail]>
on that as that seems to be the right thing to do.

And there should be no issues in placing a i915_gem_set_wedged()
immediately after the call to i915_driver_unregister, so if you include
a line of commentary about why, for example

/*
 * After unregistering the device to prevent any new users, cancel
 * all in-flight requests so that we can quickly unbind the active
 * resources.
 */
i915_gem_set_wedged(dev_priv);

Reviewed-by: Chris Wilson <[e-mail]>

I think overall though, we need to go through i915_driver_unload() and
push the module cleanup operations to i915_driver_release -- that will
take a bit of surgery to separate the different phases that are
currently smashed together.
-Chris

Janusz Krzysztofik April 5, 2019, 11:38 a.m. UTC | #4

On Fri, 2019-04-05 at 09:24 +0100, Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-04-05 09:11:54)
> > On Fri, 2019-04-05 at 08:41 +0100, Chris Wilson wrote:
> > > Quoting Janusz Krzysztofik (2019-04-05 08:26:57)
> > > > From: Janusz Krzysztofik <[e-mail]>
> > > > 
> > > > 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 <[e-mail]
> > > > >
> > > > ---
> > > >  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.
> > 
> > OK, thanks.  Do you prefer them squashed or as serparate patches?
> 
> Quite happy to do the s/unregister/unplug/ and move in one go. Have a
> pre-emptive
> Reviewed-by: Chris Wilson <[e-mail]>
> on that as that seems to be the right thing to do.
> 
> And there should be no issues in placing a i915_gem_set_wedged()
> immediately after the call to i915_driver_unregister, so if you
> include
> a line of commentary about why, for example
> 
> /*
>  * After unregistering the device to prevent any new users, cancel
>  * all in-flight requests so that we can quickly unbind the active
>  * resources.
>  */
> i915_gem_set_wedged(dev_priv);
> 
> Reviewed-by: Chris Wilson <[e-mail]>

I've given it some testing, no side effects with test workloads I've
tried, and looks like it at least helps to prevent from making the
device actually wedged.

With these two patches, plus the one we discussed yesterday, and yet
another one I'm going to submit soon, I'm now able to unbind the driver
from a device while a workload is running on it, unload the module,
reload it and successfully perform basic GEM health checks, all in a
quick succession :-).

Unfortunately, not 100% reproducible, as well as not the case with
device unplug simulated by writing 1 to device/remove sysfs file.
Surely that needs the work you describe below to be done first.

Thanks for your cooperation,
Janusz


> 
> I think overall though, we need to go through i915_driver_unload()
> and
> push the module cleanup operations to i915_driver_release -- that
> will
> take a bit of surgery to separate the different phases that are
> currently smashed together.
> -Chris
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter April 15, 2019, 9:29 a.m. UTC | #5

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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

Patch

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);
 
 	i915_gem_shrinker_unregister(dev_priv);
 }