| Message ID | 1465930269-7883-8-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 14, 2016 at 08:51:02PM +0200, Daniel Vetter wrote: > Another place gone where modern drivers could have hit > dev->struct_mutex. > > To avoid too deeply nesting control flow rework it a bit. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_auth.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index e015a7edb154..a58b2eb0d004 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -246,11 +246,14 @@ int drm_master_open(struct drm_file *file_priv) > void drm_master_release(struct drm_file *file_priv) > { > struct drm_device *dev = file_priv->minor->dev; > + struct drm_master *master = file_priv->master; > + Spare newline. > mutex_lock(&dev->master_mutex); > - if (file_priv->is_master) { > - struct drm_master *master = file_priv->master; > + if (!file_priv->is_master) > + goto out_unlock; > > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) { > /* > * Since the master is disappearing, so is the > * possibility to lock. > @@ -264,19 +267,20 @@ void drm_master_release(struct drm_file *file_priv) > wake_up_interruptible_all(&master->lock.lock_queue); > } > mutex_unlock(&dev->struct_mutex); > + } > > - if (file_priv->minor->master == file_priv->master) { > - /* drop the reference held my the minor */ > - if (dev->driver->master_drop) > - dev->driver->master_drop(dev, file_priv, true); > - drm_master_put(&file_priv->minor->master); > - } > + if (file_priv->minor->master == file_priv->master) { > + /* drop the reference held my the minor */ > + if (dev->driver->master_drop) > + dev->driver->master_drop(dev, file_priv, true); > + drm_master_put(&file_priv->minor->master); > } > > /* drop the master reference held by the file priv */ > if (file_priv->master) > drm_master_put(&file_priv->master); > file_priv->is_master = 0; > +out_unlock: This changes the reference counting, and from my quick scan we can have a reference on file_priv->master but file_priv->is_master == 0. -Chris
On Wed, Jun 15, 2016 at 12:48:23PM +0100, Chris Wilson wrote: > On Tue, Jun 14, 2016 at 08:51:02PM +0200, Daniel Vetter wrote: > > Another place gone where modern drivers could have hit > > dev->struct_mutex. > > > > To avoid too deeply nesting control flow rework it a bit. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_auth.c | 20 ++++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > > index e015a7edb154..a58b2eb0d004 100644 > > --- a/drivers/gpu/drm/drm_auth.c > > +++ b/drivers/gpu/drm/drm_auth.c > > @@ -246,11 +246,14 @@ int drm_master_open(struct drm_file *file_priv) > > void drm_master_release(struct drm_file *file_priv) > > { > > struct drm_device *dev = file_priv->minor->dev; > > + struct drm_master *master = file_priv->master; > > + > > Spare newline. > > > mutex_lock(&dev->master_mutex); > > - if (file_priv->is_master) { > > - struct drm_master *master = file_priv->master; > > + if (!file_priv->is_master) > > + goto out_unlock; > > > > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) { > > /* > > * Since the master is disappearing, so is the > > * possibility to lock. > > @@ -264,19 +267,20 @@ void drm_master_release(struct drm_file *file_priv) > > wake_up_interruptible_all(&master->lock.lock_queue); > > } > > mutex_unlock(&dev->struct_mutex); > > + } > > > > - if (file_priv->minor->master == file_priv->master) { > > - /* drop the reference held my the minor */ > > - if (dev->driver->master_drop) > > - dev->driver->master_drop(dev, file_priv, true); > > - drm_master_put(&file_priv->minor->master); > > - } > > + if (file_priv->minor->master == file_priv->master) { > > + /* drop the reference held my the minor */ > > + if (dev->driver->master_drop) > > + dev->driver->master_drop(dev, file_priv, true); > > + drm_master_put(&file_priv->minor->master); > > } > > > > /* drop the master reference held by the file priv */ > > if (file_priv->master) > > drm_master_put(&file_priv->master); > > file_priv->is_master = 0; > > +out_unlock: > > This changes the reference counting, and from my quick scan we can have > a reference on file_priv->master but file_priv->is_master == 0. Indeed, I've created a leak. Will fix. -Daniel
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index e015a7edb154..a58b2eb0d004 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -246,11 +246,14 @@ int drm_master_open(struct drm_file *file_priv) void drm_master_release(struct drm_file *file_priv) { struct drm_device *dev = file_priv->minor->dev; + struct drm_master *master = file_priv->master; + mutex_lock(&dev->master_mutex); - if (file_priv->is_master) { - struct drm_master *master = file_priv->master; + if (!file_priv->is_master) + goto out_unlock; + if (!drm_core_check_feature(dev, DRIVER_MODESET)) { /* * Since the master is disappearing, so is the * possibility to lock. @@ -264,19 +267,20 @@ void drm_master_release(struct drm_file *file_priv) wake_up_interruptible_all(&master->lock.lock_queue); } mutex_unlock(&dev->struct_mutex); + } - if (file_priv->minor->master == file_priv->master) { - /* drop the reference held my the minor */ - if (dev->driver->master_drop) - dev->driver->master_drop(dev, file_priv, true); - drm_master_put(&file_priv->minor->master); - } + if (file_priv->minor->master == file_priv->master) { + /* drop the reference held my the minor */ + if (dev->driver->master_drop) + dev->driver->master_drop(dev, file_priv, true); + drm_master_put(&file_priv->minor->master); } /* drop the master reference held by the file priv */ if (file_priv->master) drm_master_put(&file_priv->master); file_priv->is_master = 0; +out_unlock: mutex_unlock(&dev->master_mutex); }
Another place gone where modern drivers could have hit dev->struct_mutex. To avoid too deeply nesting control flow rework it a bit. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_auth.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)