Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 15 Apr 2019 11:29:27 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Chris Wilson <chris@chris-wilson.co.uk>
Cc:     Jani Nikula <jani.nikula@linux.intel.com>,
        Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, michal.wajdeczko@intel.com,
        intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org,
        Janusz Krzysztofik <janusz.krzysztofik@intel.com>
Subject: Re: [PATCH] drm/i915: Use drm_dev_unplug()
Message-ID: <20190415092926.GW2665@phenom.ffwll.local>
Mail-Followup-To: Chris Wilson <chris@chris-wilson.co.uk>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        David Airlie <airlied@linux.ie>, michal.wajdeczko@intel.com,
        intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org,
        Janusz Krzysztofik <janusz.krzysztofik@intel.com>
References: <20190405072657.9997-1-janusz.krzysztofik@linux.intel.com>
 <155445007634.8259.18397004914881394541@skylake-alporthouse-com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <155445007634.8259.18397004914881394541@skylake-alporthouse-com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@intel.com>
> > 
> > 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@intel.com>
> > ---
> >  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