| Message ID | 20191112175048.1581-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/fb-helper: unexpoert drm_fb_helper_generic_probe | expand |
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); >
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); > >
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);
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(+)