[2/2] drm/fb-helper: refcount drm_device for generic support
diff mbox series

Message ID 20191112175048.1581-2-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/2] drm/fb-helper: unexpoert drm_fb_helper_generic_probe
Related show

Commit Message

Daniel Vetter Nov. 12, 2019, 5:50 p.m. UTC
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@redhat.com>
Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Daniel Vetter <daniel.vetter@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.
-Daniel
---
 drivers/gpu/drm/drm_fb_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Noralf Trønnes Nov. 12, 2019, 8:57 p.m. UTC | #1
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@redhat.com>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Daniel Vetter <daniel.vetter@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);
>
Daniel Vetter Nov. 12, 2019, 9:01 p.m. UTC | #2
On Tue, Nov 12, 2019 at 9:57 PM Noralf Trønnes <noralf@tronnes.org> wrote:
> 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@redhat.com>
> > Cc: Noralf Trønnes <noralf@tronnes.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.

Oh nice, I totally missed that.

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

Yeah that part I got, but I missed that drm_client is holding a ref on
the drm_device until drm_client_release time. I somehow thought we
still had an issue there between fbdev references and drm_device
teardown. But it's all fixed already.

I'll merge the first patch and drop this one, thanks for your review.
-Daniel

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

Patch
diff mbox series

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