On Thu, May 05, 2022 at 09:29:40AM +0200, Thomas Zimmermann wrote: > Hi > > Am 04.05.22 um 23:57 schrieb Javier Martinez Canillas: > > The driver is calling framebuffer_release() in its .remove callback, but > > this will cause the struct fb_info to be freed too early. Since it could > > be that a reference is still hold to it if user-space opened the fbdev. > > > > This would lead to a use-after-free error if the framebuffer device was > > unregistered but later a user-space process tries to close the fbdev fd. > > > > The correct thing to do is to only unregister the framebuffer in the > > driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy. > > > > Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> > > Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx> > > Please see my question below. > > > --- > > > > drivers/video/fbdev/simplefb.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c > > index 94fc9c6d0411..2c198561c338 100644 > > --- a/drivers/video/fbdev/simplefb.c > > +++ b/drivers/video/fbdev/simplefb.c > > @@ -84,6 +84,10 @@ struct simplefb_par { > > static void simplefb_clocks_destroy(struct simplefb_par *par); > > static void simplefb_regulators_destroy(struct simplefb_par *par); > > +/* > > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end > > + * of unregister_framebuffer() or fb_release(). Do any cleanup here. > > + */ > > static void simplefb_destroy(struct fb_info *info) > > { > > struct simplefb_par *par = info->par; > > @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info) > > if (info->screen_base) > > iounmap(info->screen_base); > > + framebuffer_release(info); > > + > > if (mem) > > release_mem_region(mem->start, resource_size(mem)); > > The original problem with fbdev hot-unplug was that vmwgfx needed the > framebuffer region to be released. If we release it only after userspace > closed it's final file descriptor, vmwgfx could have already failed. > > I still don't fully get why this code apparently works or at least doesn't > blow up occasionally. Any ideas? See my other reply, releasing hw stuff should be done from ->remove, not ->fb_destroy. Also note that none of the patches discussed moved around anything here, we simply leaked things a bit when only unregistering the fb and not going through the device->remove callback. I guess in practice it works because unregistering the device sends out a uevent, and userspace then closes all it's device fd as a reaction to that, and usually that's fast enough to not upset anyone? No idea tbh. -Daniel > Best regards > Thomas > > > } > > @@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev) > > { > > struct fb_info *info = platform_get_drvdata(pdev); > > + /* simplefb_destroy takes care of info cleanup */ > > unregister_framebuffer(info); > > - framebuffer_release(info); > > return 0; > > } > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch