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@xxxxxxxxx>
> > ---
> > 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
