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

[Daniel Vetter]

On Tue, Nov 12, 2019 at 9:57 PM Noralf Trønnes <noralf at 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 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.

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch