[PATCH 2/2] drm/fb-helper: refcount drm_device for generic support

[Noralf Trønnes]

Den 12.11.2019 18.50, skrev Daniel Vetter:
> There's a pile of things protecting against racing hotunplugs:
> - drm_dev_enter/exit against the underlying device disappearing
> - drm_dev_get/put against drm_device disappearing
> - we unregister everything in drm_dev_unregister to stop userspace from
>   creating new open files to access the drm_device
> - fbcon gets synchronously torn down (the console_lock is huge, so there's
>   an upside to monolithic locking here).
> - and for fbdev there's get_fb_info and put_fb_info
> 
> And I think we've almost got this right, except for a tiny thing: An fbdev
> ioctl might hold an elevate fb_info reference, which prevents us from
> doing the final cleanup in drm_fbdev_release. But it doesn't prevent
> the final cleanup of fbhelper->dev, which means even if the driver protects
> all hw access with drm_dev_enter/exit, we might oops way before that when
> trying to access freed memory.
> 
> Fix this by holding a drm_device reference for fbdev, which we drop from
> drm_fbdev_release.
> 
> This might lead to a reference loop, where the drm_device won't get cleaned
> up because of the fbdev reference, and fbdev isn't torn down yet because
> the drm_device cleanup code is written the wrong way round. But I think
> all drivers using the generic fbdev code get this right (because they use
> drm_client, so really can't get it wrong), hence this should be ok.
> 
> Also, more reasons to switch drivers over to generic fbdev support.
> 
> Since I had to re-review a pile of code also add a comment about why
> drm_fbdev_client_unregister can't race.
> 
> Cc: Gerd Hoffmann <kraxel at redhat.com>
> Cc: Noralf Trønnes <noralf at tronnes.org>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> --
> Entirely untested (aside from it compiles), but I think we need
> something like this to completely plug all fbdev related hotunplug
> issues. No need to roll out some fancy srcu scheme to fbdev, this
> seems to be solved with refcounting already.

We already hold a ref on drm_device taken in drm_client_init().
Hotunplug is working for generic fbdev, fbcon is automatically unbound
and if userspace is open teardown is delayed until the fd is closed.

This comment here gives some hints:

/*
 * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of
 * unregister_framebuffer() or fb_release().
 */
static void drm_fbdev_fb_destroy(struct fb_info *info)
{
	drm_fbdev_release(info->par);
}

Noralf.

> -Daniel
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0ec98e046b59..62a1981d369e 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1992,6 +1992,7 @@ static void drm_fbdev_cleanup(struct drm_fb_helper *fb_helper)
>  
>  static void drm_fbdev_release(struct drm_fb_helper *fb_helper)
>  {
> +	drm_dev_put(fb_helper->dev);
>  	drm_fbdev_cleanup(fb_helper);
>  	drm_client_release(&fb_helper->client);
>  	kfree(fb_helper);
> @@ -2123,6 +2124,7 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client)
>  {
>  	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>  
> +	/* Protected against races by the clientlist_mutex. */
>  	if (fb_helper->fbdev)
>  		/* drm_fbdev_fb_destroy() takes care of cleanup */
>  		drm_fb_helper_unregister_fbi(fb_helper);
> @@ -2243,6 +2245,8 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  		preferred_bpp = 32;
>  	fb_helper->preferred_bpp = preferred_bpp;
>  
> +	drm_dev_get(dev);
> +
>  	ret = drm_fbdev_client_hotplug(&fb_helper->client);
>  	if (ret)
>  		DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
>