[Intel-gfx] [PATCH 21/51] drm: Use drmm_ for drm_dev_init cleanup

[Thomas Zimmermann]

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 at 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);
>  
> 

-- 
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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20200311/9883eda5/attachment.sig>