diff mbox series

[21/51] drm: Use drmm_ for drm_dev_init cleanup

Message ID 20200302222631.3861340-22-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show
Series drm_device managed resources, v4 | expand

Commit Message

Daniel Vetter March 2, 2020, 10:26 p.m. UTC 
Well for the simple stuff at least, vblank, gem and minor cleanup I
want to further split up as a demonstration.

v2: We need to clear drm_device->dev otherwise the debug drm printing
after our cleanup hook (e.g. in drm_manged_release) will chase
released memory and result in a use-after-free. Not really pretty, but
oh well.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 48 ++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

Comments

Thomas Zimmermann March 11, 2020, 9:39 a.m. UTC | #1 
Hi

Am 02.03.20 um 23:26 schrieb Daniel Vetter:
> Well for the simple stuff at least, vblank, gem and minor cleanup I
> want to further split up as a demonstration.
> 
> v2: We need to clear drm_device->dev otherwise the debug drm printing
> after our cleanup hook (e.g. in drm_manged_release) will chase
> released memory and result in a use-after-free. Not really pretty, but
> oh well.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 48 ++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index ef79c03e311c..23e5b0e7e041 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -580,6 +580,23 @@ static void drm_fs_inode_free(struct inode *inode)
>   *    used.
>   */
>  
> +static void drm_dev_init_release(struct drm_device *dev, void *res)
> +{
> +	drm_legacy_ctxbitmap_cleanup(dev);
> +	drm_legacy_remove_map_hash(dev);
> +	drm_fs_inode_free(dev->anon_inode);
> +
> +	put_device(dev->dev);
> +	/* Prevent use-after-free in drm_managed_release when debugging is
> +	 * enabled. Slightly awkward, but can't really be helped. */
> +	dev->dev = NULL;
> +	mutex_destroy(&dev->master_mutex);
> +	mutex_destroy(&dev->clientlist_mutex);
> +	mutex_destroy(&dev->filelist_mutex);
> +	mutex_destroy(&dev->struct_mutex);
> +	drm_legacy_destroy_members(dev);
> +}
> +
>  /**
>   * drm_dev_init - Initialise new DRM device
>   * @dev: DRM device
> @@ -647,11 +664,15 @@ int drm_dev_init(struct drm_device *dev,
>  	mutex_init(&dev->clientlist_mutex);
>  	mutex_init(&dev->master_mutex);
>  
> +	ret = drmm_add_action(dev, drm_dev_init_release, NULL);
> +	if (ret)
> +		return ret;
> +

Is this code supposed to stay for the long term? As devices are
allocated dynamically, I can imagine that there will be a call that
allocates the memory and, at the same time, sets drm_dev_init_release()
as the release callback.

The question is also released to patch 3, where I proposed to rename
__drm_add_action() to __drmm_kzalloc().

>  	dev->anon_inode = drm_fs_inode_new();
>  	if (IS_ERR(dev->anon_inode)) {
>  		ret = PTR_ERR(dev->anon_inode);
>  		DRM_ERROR("Cannot allocate anonymous inode: %d\n", ret);
> -		goto err_free;
> +		goto err;
>  	}
>  
>  	if (drm_core_check_feature(dev, DRIVER_RENDER)) {
> @@ -688,19 +709,12 @@ int drm_dev_init(struct drm_device *dev,
>  	if (drm_core_check_feature(dev, DRIVER_GEM))
>  		drm_gem_destroy(dev);
>  err_ctxbitmap:
> -	drm_legacy_ctxbitmap_cleanup(dev);
> -	drm_legacy_remove_map_hash(dev);
>  err_minors:
>  	drm_minor_free(dev, DRM_MINOR_PRIMARY);
>  	drm_minor_free(dev, DRM_MINOR_RENDER);
> -	drm_fs_inode_free(dev->anon_inode);
> -err_free:
> -	put_device(dev->dev);
> -	mutex_destroy(&dev->master_mutex);
> -	mutex_destroy(&dev->clientlist_mutex);
> -	mutex_destroy(&dev->filelist_mutex);
> -	mutex_destroy(&dev->struct_mutex);
> -	drm_legacy_destroy_members(dev);
> +err:
> +	drm_managed_release(dev);
> +

Here's more of a general observation than a comment on the actual patch:

One odd thing about the overall interface is that there's no way of
updating the release callback afterwards. In an OOP language, such as
C++, an error within the constructor would rollback the performed
actions and return without calling the destructor. Destructors only run
for fully constructed objects.

In our case, the equivalent is to run the init function and set
drm_dev_init_release() as the final step. The init's rollback-code would
have to stay, obviously.

Best regards
Thomas

>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_dev_init);
> @@ -763,20 +777,8 @@ void drm_dev_fini(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_GEM))
>  		drm_gem_destroy(dev);
>  
> -	drm_legacy_ctxbitmap_cleanup(dev);
> -	drm_legacy_remove_map_hash(dev);
> -	drm_fs_inode_free(dev->anon_inode);
> -
>  	drm_minor_free(dev, DRM_MINOR_PRIMARY);
>  	drm_minor_free(dev, DRM_MINOR_RENDER);
> -
> -	put_device(dev->dev);
> -
> -	mutex_destroy(&dev->master_mutex);
> -	mutex_destroy(&dev->clientlist_mutex);
> -	mutex_destroy(&dev->filelist_mutex);
> -	mutex_destroy(&dev->struct_mutex);
> -	drm_legacy_destroy_members(dev);
>  }
>  EXPORT_SYMBOL(drm_dev_fini);
>  
>
Daniel Vetter March 16, 2020, 9:02 a.m. UTC | #2 
On Wed, Mar 11, 2020 at 10:39:13AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 02.03.20 um 23:26 schrieb Daniel Vetter:
> > Well for the simple stuff at least, vblank, gem and minor cleanup I
> > want to further split up as a demonstration.
> > 
> > v2: We need to clear drm_device->dev otherwise the debug drm printing
> > after our cleanup hook (e.g. in drm_manged_release) will chase
> > released memory and result in a use-after-free. Not really pretty, but
> > oh well.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_drv.c | 48 ++++++++++++++++++++-------------------
> >  1 file changed, 25 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index ef79c03e311c..23e5b0e7e041 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -580,6 +580,23 @@ static void drm_fs_inode_free(struct inode *inode)
> >   *    used.
> >   */
> >  
> > +static void drm_dev_init_release(struct drm_device *dev, void *res)
> > +{
> > +	drm_legacy_ctxbitmap_cleanup(dev);
> > +	drm_legacy_remove_map_hash(dev);
> > +	drm_fs_inode_free(dev->anon_inode);
> > +
> > +	put_device(dev->dev);
> > +	/* Prevent use-after-free in drm_managed_release when debugging is
> > +	 * enabled. Slightly awkward, but can't really be helped. */
> > +	dev->dev = NULL;
> > +	mutex_destroy(&dev->master_mutex);
> > +	mutex_destroy(&dev->clientlist_mutex);
> > +	mutex_destroy(&dev->filelist_mutex);
> > +	mutex_destroy(&dev->struct_mutex);
> > +	drm_legacy_destroy_members(dev);
> > +}
> > +
> >  /**
> >   * drm_dev_init - Initialise new DRM device
> >   * @dev: DRM device
> > @@ -647,11 +664,15 @@ int drm_dev_init(struct drm_device *dev,
> >  	mutex_init(&dev->clientlist_mutex);
> >  	mutex_init(&dev->master_mutex);
> >  
> > +	ret = drmm_add_action(dev, drm_dev_init_release, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> 
> Is this code supposed to stay for the long term? As devices are
> allocated dynamically, I can imagine that there will be a call that
> allocates the memory and, at the same time, sets drm_dev_init_release()
> as the release callback.

There's a chicken-egg situation here. The plan is to fix this with a
devm_drm_dev_alloc() macro, which we discussed quite a bit in earlier
versions. It's just that the patch series is already big as-is, hence this
is postponed to the next round.

> The question is also released to patch 3, where I proposed to rename
> __drm_add_action() to __drmm_kzalloc().
> 
> >  	dev->anon_inode = drm_fs_inode_new();
> >  	if (IS_ERR(dev->anon_inode)) {
> >  		ret = PTR_ERR(dev->anon_inode);
> >  		DRM_ERROR("Cannot allocate anonymous inode: %d\n", ret);
> > -		goto err_free;
> > +		goto err;
> >  	}
> >  
> >  	if (drm_core_check_feature(dev, DRIVER_RENDER)) {
> > @@ -688,19 +709,12 @@ int drm_dev_init(struct drm_device *dev,
> >  	if (drm_core_check_feature(dev, DRIVER_GEM))
> >  		drm_gem_destroy(dev);
> >  err_ctxbitmap:
> > -	drm_legacy_ctxbitmap_cleanup(dev);
> > -	drm_legacy_remove_map_hash(dev);
> >  err_minors:
> >  	drm_minor_free(dev, DRM_MINOR_PRIMARY);
> >  	drm_minor_free(dev, DRM_MINOR_RENDER);
> > -	drm_fs_inode_free(dev->anon_inode);
> > -err_free:
> > -	put_device(dev->dev);
> > -	mutex_destroy(&dev->master_mutex);
> > -	mutex_destroy(&dev->clientlist_mutex);
> > -	mutex_destroy(&dev->filelist_mutex);
> > -	mutex_destroy(&dev->struct_mutex);
> > -	drm_legacy_destroy_members(dev);
> > +err:
> > +	drm_managed_release(dev);
> > +
> 
> Here's more of a general observation than a comment on the actual patch:
> 
> One odd thing about the overall interface is that there's no way of
> updating the release callback afterwards. In an OOP language, such as
> C++, an error within the constructor would rollback the performed
> actions and return without calling the destructor. Destructors only run
> for fully constructed objects.
> 
> In our case, the equivalent is to run the init function and set
> drm_dev_init_release() as the final step. The init's rollback-code would
> have to stay, obviously.

See the various drivers later on in the series, the init rollback
completely disappears once we're done here. If this wouldn't Just Work for
both final destruction and init rollback there's really no point. There's
a few ugly corner-cases with getting this boot-strapped though.
-Daniel

> 
> Best regards
> Thomas
> 
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL(drm_dev_init);
> > @@ -763,20 +777,8 @@ void drm_dev_fini(struct drm_device *dev)
> >  	if (drm_core_check_feature(dev, DRIVER_GEM))
> >  		drm_gem_destroy(dev);
> >  
> > -	drm_legacy_ctxbitmap_cleanup(dev);
> > -	drm_legacy_remove_map_hash(dev);
> > -	drm_fs_inode_free(dev->anon_inode);
> > -
> >  	drm_minor_free(dev, DRM_MINOR_PRIMARY);
> >  	drm_minor_free(dev, DRM_MINOR_RENDER);
> > -
> > -	put_device(dev->dev);
> > -
> > -	mutex_destroy(&dev->master_mutex);
> > -	mutex_destroy(&dev->clientlist_mutex);
> > -	mutex_destroy(&dev->filelist_mutex);
> > -	mutex_destroy(&dev->struct_mutex);
> > -	drm_legacy_destroy_members(dev);
> >  }
> >  EXPORT_SYMBOL(drm_dev_fini);
> >  
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index ef79c03e311c..23e5b0e7e041 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -580,6 +580,23 @@  static void drm_fs_inode_free(struct inode *inode)
  *    used.
  */
 
+static void drm_dev_init_release(struct drm_device *dev, void *res)
+{
+	drm_legacy_ctxbitmap_cleanup(dev);
+	drm_legacy_remove_map_hash(dev);
+	drm_fs_inode_free(dev->anon_inode);
+
+	put_device(dev->dev);
+	/* Prevent use-after-free in drm_managed_release when debugging is
+	 * enabled. Slightly awkward, but can't really be helped. */
+	dev->dev = NULL;
+	mutex_destroy(&dev->master_mutex);
+	mutex_destroy(&dev->clientlist_mutex);
+	mutex_destroy(&dev->filelist_mutex);
+	mutex_destroy(&dev->struct_mutex);
+	drm_legacy_destroy_members(dev);
+}
+
 /**
  * drm_dev_init - Initialise new DRM device
  * @dev: DRM device
@@ -647,11 +664,15 @@  int drm_dev_init(struct drm_device *dev,
 	mutex_init(&dev->clientlist_mutex);
 	mutex_init(&dev->master_mutex);
 
+	ret = drmm_add_action(dev, drm_dev_init_release, NULL);
+	if (ret)
+		return ret;
+
 	dev->anon_inode = drm_fs_inode_new();
 	if (IS_ERR(dev->anon_inode)) {
 		ret = PTR_ERR(dev->anon_inode);
 		DRM_ERROR("Cannot allocate anonymous inode: %d\n", ret);
-		goto err_free;
+		goto err;
 	}
 
 	if (drm_core_check_feature(dev, DRIVER_RENDER)) {
@@ -688,19 +709,12 @@  int drm_dev_init(struct drm_device *dev,
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_destroy(dev);
 err_ctxbitmap:
-	drm_legacy_ctxbitmap_cleanup(dev);
-	drm_legacy_remove_map_hash(dev);
 err_minors:
 	drm_minor_free(dev, DRM_MINOR_PRIMARY);
 	drm_minor_free(dev, DRM_MINOR_RENDER);
-	drm_fs_inode_free(dev->anon_inode);
-err_free:
-	put_device(dev->dev);
-	mutex_destroy(&dev->master_mutex);
-	mutex_destroy(&dev->clientlist_mutex);
-	mutex_destroy(&dev->filelist_mutex);
-	mutex_destroy(&dev->struct_mutex);
-	drm_legacy_destroy_members(dev);
+err:
+	drm_managed_release(dev);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_dev_init);
@@ -763,20 +777,8 @@  void drm_dev_fini(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_destroy(dev);
 
-	drm_legacy_ctxbitmap_cleanup(dev);
-	drm_legacy_remove_map_hash(dev);
-	drm_fs_inode_free(dev->anon_inode);
-
 	drm_minor_free(dev, DRM_MINOR_PRIMARY);
 	drm_minor_free(dev, DRM_MINOR_RENDER);
-
-	put_device(dev->dev);
-
-	mutex_destroy(&dev->master_mutex);
-	mutex_destroy(&dev->clientlist_mutex);
-	mutex_destroy(&dev->filelist_mutex);
-	mutex_destroy(&dev->struct_mutex);
-	drm_legacy_destroy_members(dev);
 }
 EXPORT_SYMBOL(drm_dev_fini);