| Message ID | 20200723172119.17649-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm: Restore driver.preclose() for all to use | expand |
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Thursday, July 23, 2020 10:21 AM > To: intel-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org; Chris Wilson <chris@chris-wilson.co.uk>; > Tang, CQ <cq.tang@intel.com>; Vetter, Daniel <daniel.vetter@intel.com>; > stable@vger.kernel.org > Subject: [PATCH 2/3] drm/i915/gem: Move context decoupling from > postclose to preclose > > Since the GEM contexts refer to other GEM state, we need to nerf those > pointers before that state is freed during drm_gem_release(). We need to > move i915_gem_context_close() from the postclose callback to the preclose. > > In particular, debugfs likes to peek into the GEM contexts, and from there > peek at the drm core objects. If the context is closed during the peeking, we > may attempt to dereference a stale core object. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: CQ Tang <cq.tang@intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_drv.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c index 5fd5af4bc855..15242a8c70f7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1114,11 +1114,15 @@ static void i915_driver_lastclose(struct > drm_device *dev) > vga_switcheroo_process_delayed_switch(); > } > > +static void i915_driver_preclose(struct drm_device *dev, struct > +drm_file *file) { > + i915_gem_context_close(file); > +} > + > static void i915_driver_postclose(struct drm_device *dev, struct drm_file > *file) { > struct drm_i915_file_private *file_priv = file->driver_priv; > > - i915_gem_context_close(file); > i915_gem_release(dev, file); Now we separate i915_gem_context_close() from i915_gem_release() and other freeing code in postclose(), is there any side effect to allow code to run in between? Can we move all postclose() code into preclose()? --CQ > > kfree_rcu(file_priv, rcu); > @@ -1850,6 +1854,7 @@ static struct drm_driver driver = { > .release = i915_driver_release, > .open = i915_driver_open, > .lastclose = i915_driver_lastclose, > + .preclose = i915_driver_preclose, > .postclose = i915_driver_postclose, > > .gem_close_object = i915_gem_close_object, > -- > 2.20.1
Quoting Tang, CQ (2020-07-23 18:44:08) > > > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Thursday, July 23, 2020 10:21 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: dri-devel@lists.freedesktop.org; Chris Wilson <chris@chris-wilson.co.uk>; > > Tang, CQ <cq.tang@intel.com>; Vetter, Daniel <daniel.vetter@intel.com>; > > stable@vger.kernel.org > > Subject: [PATCH 2/3] drm/i915/gem: Move context decoupling from > > postclose to preclose > > > > Since the GEM contexts refer to other GEM state, we need to nerf those > > pointers before that state is freed during drm_gem_release(). We need to > > move i915_gem_context_close() from the postclose callback to the preclose. > > > > In particular, debugfs likes to peek into the GEM contexts, and from there > > peek at the drm core objects. If the context is closed during the peeking, we > > may attempt to dereference a stale core object. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: CQ Tang <cq.tang@intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: stable@vger.kernel.org > > --- > > drivers/gpu/drm/i915/i915_drv.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c index 5fd5af4bc855..15242a8c70f7 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1114,11 +1114,15 @@ static void i915_driver_lastclose(struct > > drm_device *dev) > > vga_switcheroo_process_delayed_switch(); > > } > > > > +static void i915_driver_preclose(struct drm_device *dev, struct > > +drm_file *file) { > > + i915_gem_context_close(file); > > +} > > + > > static void i915_driver_postclose(struct drm_device *dev, struct drm_file > > *file) { > > struct drm_i915_file_private *file_priv = file->driver_priv; > > > > - i915_gem_context_close(file); > > i915_gem_release(dev, file); > > Now we separate i915_gem_context_close() from i915_gem_release() and other freeing code in postclose(), is there any side effect to allow code to run in between? > Can we move all postclose() code into preclose()? i915_gem_release() is scheduled for deletion, so I didn't care. What remains in postclose are the kfree + tidyup, which seem like a good idea to keep last. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 5fd5af4bc855..15242a8c70f7 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1114,11 +1114,15 @@ static void i915_driver_lastclose(struct drm_device *dev) vga_switcheroo_process_delayed_switch(); } +static void i915_driver_preclose(struct drm_device *dev, struct drm_file *file) +{ + i915_gem_context_close(file); +} + static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) { struct drm_i915_file_private *file_priv = file->driver_priv; - i915_gem_context_close(file); i915_gem_release(dev, file); kfree_rcu(file_priv, rcu); @@ -1850,6 +1854,7 @@ static struct drm_driver driver = { .release = i915_driver_release, .open = i915_driver_open, .lastclose = i915_driver_lastclose, + .preclose = i915_driver_preclose, .postclose = i915_driver_postclose, .gem_close_object = i915_gem_close_object,
Since the GEM contexts refer to other GEM state, we need to nerf those pointers before that state is freed during drm_gem_release(). We need to move i915_gem_context_close() from the postclose callback to the preclose. In particular, debugfs likes to peek into the GEM contexts, and from there peek at the drm core objects. If the context is closed during the peeking, we may attempt to dereference a stale core object. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: CQ Tang <cq.tang@intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/i915/i915_drv.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)