From 712f3147aee0fbbbbed2da20b21b272c5505125e Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Fri, 13 May 2011 16:16:41 -0700 Subject: fbmem: fix remove_conflicting_framebuffers races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a register_framebuffer() call results in us removing old conflicting framebuffers, the new registration_lock doesn't protect that situation. And we can't just add the same locking to the function, because these functions call each other: register_framebuffer() calls remove_conflicting_framebuffers, which in turn calls unregister_framebuffer for any conflicting entry. In order to fix it, this just creates wrapper functions around all three functions and makes the versions that actually do the work be called "do_xxx()", leaving just the wrapper that gets the lock and calls the worker function. So the rule becomes simply that "do_xxxx()" has to be called with the lock held, and now do_register_framebuffer() can just call do_remove_conflicting_framebuffers(), and that in turn can call _do_unregister_framebuffer(), and there is no deadlock, and we can hold the registration lock over the whole sequence, fixing the races. It also makes error cases simpler, and fixes one situation where we would return from unregister_framebuffer() without releasing the lock, pointed out by Bruno Prémont. Tested-by: Bruno Prémont Tested-by: Anca Emanuel Signed-off-by: Linus Torvalds --- drivers/video/fbmem.c | 117 +++++++++++++++++++++++++++++--------------------- 1 file changed, 68 insertions(+), 49 deletions(-) diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c index ea16e654a9b..46ee5e5a08c 100644 --- a/drivers/video/fbmem.c +++ b/drivers/video/fbmem.c @@ -1537,8 +1537,10 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena, return false; } +static int do_unregister_framebuffer(struct fb_info *fb_info); + #define VGA_FB_PHYS 0xA0000 -void remove_conflicting_framebuffers(struct apertures_struct *a, +static void do_remove_conflicting_framebuffers(struct apertures_struct *a, const char *name, bool primary) { int i; @@ -1560,24 +1562,12 @@ void remove_conflicting_framebuffers(struct apertures_struct *a, printk(KERN_INFO "fb: conflicting fb hw usage " "%s vs %s - removing generic driver\n", name, registered_fb[i]->fix.id); - unregister_framebuffer(registered_fb[i]); + do_unregister_framebuffer(registered_fb[i]); } } } -EXPORT_SYMBOL(remove_conflicting_framebuffers); - -/** - * register_framebuffer - registers a frame buffer device - * @fb_info: frame buffer info structure - * - * Registers a frame buffer device @fb_info. - * - * Returns negative errno on error, or zero for success. - * - */ -int -register_framebuffer(struct fb_info *fb_info) +static int do_register_framebuffer(struct fb_info *fb_info) { int i; struct fb_event event; @@ -1589,10 +1579,9 @@ register_framebuffer(struct fb_info *fb_info) if (fb_check_foreignness(fb_info)) return -ENOSYS; - remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id, + do_remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id, fb_is_primary_device(fb_info)); - mutex_lock(®istration_lock); num_registered_fb++; for (i = 0 ; i < FB_MAX; i++) if (!registered_fb[i]) @@ -1635,7 +1624,6 @@ register_framebuffer(struct fb_info *fb_info) fb_var_to_videomode(&mode, &fb_info->var); fb_add_videomode(&mode, &fb_info->modelist); registered_fb[i] = fb_info; - mutex_unlock(®istration_lock); event.info = fb_info; if (!lock_fb_info(fb_info)) @@ -1645,37 +1633,14 @@ register_framebuffer(struct fb_info *fb_info) return 0; } - -/** - * unregister_framebuffer - releases a frame buffer device - * @fb_info: frame buffer info structure - * - * Unregisters a frame buffer device @fb_info. - * - * Returns negative errno on error, or zero for success. - * - * This function will also notify the framebuffer console - * to release the driver. - * - * This is meant to be called within a driver's module_exit() - * function. If this is called outside module_exit(), ensure - * that the driver implements fb_open() and fb_release() to - * check that no processes are using the device. - */ - -int -unregister_framebuffer(struct fb_info *fb_info) +static int do_unregister_framebuffer(struct fb_info *fb_info) { struct fb_event event; int i, ret = 0; - mutex_lock(®istration_lock); i = fb_info->node; - if (!registered_fb[i]) { - ret = -EINVAL; - goto done; - } - + if (!registered_fb[i]) + return -EINVAL; if (!lock_fb_info(fb_info)) return -ENODEV; @@ -1683,10 +1648,8 @@ unregister_framebuffer(struct fb_info *fb_info) ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event); unlock_fb_info(fb_info); - if (ret) { - ret = -EINVAL; - goto done; - } + if (ret) + return -EINVAL; if (fb_info->pixmap.addr && (fb_info->pixmap.flags & FB_PIXMAP_DEFAULT)) @@ -1701,8 +1664,64 @@ unregister_framebuffer(struct fb_info *fb_info) /* this may free fb info */ put_fb_info(fb_info); -done: + return 0; +} + +void remove_conflicting_framebuffers(struct apertures_struct *a, + const char *name, bool primary) +{ + mutex_lock(®istration_lock); + do_remove_conflicting_framebuffers(a, name, primary); mutex_unlock(®istration_lock); +} +EXPORT_SYMBOL(remove_conflicting_framebuffers); + +/** + * register_framebuffer - registers a frame buffer device + * @fb_info: frame buffer info structure + * + * Registers a frame buffer device @fb_info. + * + * Returns negative errno on error, or zero for success. + * + */ +int +register_framebuffer(struct fb_info *fb_info) +{ + int ret; + + mutex_lock(®istration_lock); + ret = do_register_framebuffer(fb_info); + mutex_unlock(®istration_lock); + + return ret; +} + +/** + * unregister_framebuffer - releases a frame buffer device + * @fb_info: frame buffer info structure + * + * Unregisters a frame buffer device @fb_info. + * + * Returns negative errno on error, or zero for success. + * + * This function will also notify the framebuffer console + * to release the driver. + * + * This is meant to be called within a driver's module_exit() + * function. If this is called outside module_exit(), ensure + * that the driver implements fb_open() and fb_release() to + * check that no processes are using the device. + */ +int +unregister_framebuffer(struct fb_info *fb_info) +{ + int ret; + + mutex_lock(®istration_lock); + ret = do_unregister_framebuffer(fb_info); + mutex_unlock(®istration_lock); + return ret; } -- cgit v1.2.3-70-g09d2 From c590cece75728a85ea06801df3ebad2d7ad8612c Mon Sep 17 00:00:00 2001 From: Bruno Prémont Date: Sat, 14 May 2011 12:24:15 +0200 Subject: Further fbcon sanity checking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This moves the if (num_registered_fb == FB_MAX) return -ENXIO; check _AFTER_ the call to do_remove_conflicting_framebuffers() as this would (now in a safe way) allow a native driver to replace the conflicting one even if all slots in registered_fb[] are taken. This also prevents unregistering a framebuffer that is no longer registered (vga16f will unregister at module unload time even if the frame buffer had been unregistered earlier due to being found conflicting). Signed-off-by: Bruno Prémont Signed-off-by: Linus Torvalds --- drivers/video/fbmem.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c index 46ee5e5a08c..5aac00eb183 100644 --- a/drivers/video/fbmem.c +++ b/drivers/video/fbmem.c @@ -1573,15 +1573,15 @@ static int do_register_framebuffer(struct fb_info *fb_info) struct fb_event event; struct fb_videomode mode; - if (num_registered_fb == FB_MAX) - return -ENXIO; - if (fb_check_foreignness(fb_info)) return -ENOSYS; do_remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id, fb_is_primary_device(fb_info)); + if (num_registered_fb == FB_MAX) + return -ENXIO; + num_registered_fb++; for (i = 0 ; i < FB_MAX; i++) if (!registered_fb[i]) @@ -1639,7 +1639,7 @@ static int do_unregister_framebuffer(struct fb_info *fb_info) int i, ret = 0; i = fb_info->node; - if (!registered_fb[i]) + if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info) return -EINVAL; if (!lock_fb_info(fb_info)) -- cgit v1.2.3-70-g09d2