| Message ID | 526DFDA8.7010806@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, Jean, How about this patch? Thanks, Gu On 10/28/2013 02:01 PM, Gu Zheng wrote: > Following commits: > 50e244cc79 fb: rework locking to fix lock ordering on takeover > e93a9a8687 fb: Yet another band-aid for fixing lockdep mess > 054430e773 fbcon: fix locking harder > > reworked locking to fix related lock ordering on takeover, and introduced console_lock > into fbmem, but it seems that the new lock sequence(fb_info->lock ---> console_lock) > is against with the one in console_callback(console_lock ---> fb_info->lock), and leads to > a potential deadlock as following: > [ 601.079000] ====================================================== > [ 601.079000] [ INFO: possible circular locking dependency detected ] > [ 601.079000] 3.11.0 #189 Not tainted > [ 601.079000] ------------------------------------------------------- > [ 601.079000] kworker/0:3/619 is trying to acquire lock: > [ 601.079000] (&fb_info->lock){+.+.+.}, at: [<ffffffff81397566>] lock_fb_info+0x26/0x60 > [ 601.079000] > but task is already holding lock: > [ 601.079000] (console_lock){+.+.+.}, at: [<ffffffff8141aae3>] console_callback+0x13/0x160 > [ 601.079000] > which lock already depends on the new lock. > > [ 601.079000] > the existing dependency chain (in reverse order) is: > [ 601.079000] > -> #1 (console_lock){+.+.+.}: > [ 601.079000] [<ffffffff810dc971>] lock_acquire+0xa1/0x140 > [ 601.079000] [<ffffffff810c6267>] console_lock+0x77/0x80 > [ 601.079000] [<ffffffff81399448>] register_framebuffer+0x1d8/0x320 > [ 601.079000] [<ffffffff81cfb4c8>] efifb_probe+0x408/0x48f > [ 601.079000] [<ffffffff8144a963>] platform_drv_probe+0x43/0x80 > [ 601.079000] [<ffffffff8144853b>] driver_probe_device+0x8b/0x390 > [ 601.079000] [<ffffffff814488eb>] __driver_attach+0xab/0xb0 > [ 601.079000] [<ffffffff814463bd>] bus_for_each_dev+0x5d/0xa0 > [ 601.079000] [<ffffffff81447e6e>] driver_attach+0x1e/0x20 > [ 601.079000] [<ffffffff81447a07>] bus_add_driver+0x117/0x290 > [ 601.079000] [<ffffffff81448fea>] driver_register+0x7a/0x170 > [ 601.079000] [<ffffffff8144a10a>] __platform_driver_register+0x4a/0x50 > [ 601.079000] [<ffffffff8144a12d>] platform_driver_probe+0x1d/0xb0 > [ 601.079000] [<ffffffff81cfb0a1>] efifb_init+0x273/0x292 > [ 601.079000] [<ffffffff81002132>] do_one_initcall+0x102/0x1c0 > [ 601.079000] [<ffffffff81cb80a6>] kernel_init_freeable+0x15d/0x1ef > [ 601.079000] [<ffffffff8166d2de>] kernel_init+0xe/0xf0 > [ 601.079000] [<ffffffff816914ec>] ret_from_fork+0x7c/0xb0 > [ 601.079000] > -> #0 (&fb_info->lock){+.+.+.}: > [ 601.079000] [<ffffffff810dc1d8>] __lock_acquire+0x1e18/0x1f10 > [ 601.079000] [<ffffffff810dc971>] lock_acquire+0xa1/0x140 > [ 601.079000] [<ffffffff816835ca>] mutex_lock_nested+0x7a/0x3b0 > [ 601.079000] [<ffffffff81397566>] lock_fb_info+0x26/0x60 > [ 601.079000] [<ffffffff813a4aeb>] fbcon_blank+0x29b/0x2e0 > [ 601.079000] [<ffffffff81418658>] do_blank_screen+0x1d8/0x280 > [ 601.079000] [<ffffffff8141ab34>] console_callback+0x64/0x160 > [ 601.079000] [<ffffffff8108d855>] process_one_work+0x1f5/0x540 > [ 601.079000] [<ffffffff8108e04c>] worker_thread+0x11c/0x370 > [ 601.079000] [<ffffffff81095fbd>] kthread+0xed/0x100 > [ 601.079000] [<ffffffff816914ec>] ret_from_fork+0x7c/0xb0 > [ 601.079000] > other info that might help us debug this: > > [ 601.079000] Possible unsafe locking scenario: > > [ 601.079000] CPU0 CPU1 > [ 601.079000] ---- ---- > [ 601.079000] lock(console_lock); > [ 601.079000] lock(&fb_info->lock); > [ 601.079000] lock(console_lock); > [ 601.079000] lock(&fb_info->lock); > [ 601.079000] > *** DEADLOCK *** > > so we reorder the lock sequence the same as it in console_callback() to > avoid this issue. > Not very sure this change is suitable, any comments is welcome. > > Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com> > --- > drivers/video/fbmem.c | 50 +++++++++++++++++++++++++++++++----------------- > 1 files changed, 32 insertions(+), 18 deletions(-) > > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c > index dacaf74..010d191 100644 > --- a/drivers/video/fbmem.c > +++ b/drivers/video/fbmem.c > @@ -1108,14 +1108,16 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > case FBIOPUT_VSCREENINFO: > if (copy_from_user(&var, argp, sizeof(var))) > return -EFAULT; > - if (!lock_fb_info(info)) > - return -ENODEV; > console_lock(); > + if (!lock_fb_info(info)) { > + console_unlock(); > + return -ENODEV; > + } > info->flags |= FBINFO_MISC_USEREVENT; > ret = fb_set_var(info, &var); > info->flags &= ~FBINFO_MISC_USEREVENT; > - console_unlock(); > unlock_fb_info(info); > + console_unlock(); > if (!ret && copy_to_user(argp, &var, sizeof(var))) > ret = -EFAULT; > break; > @@ -1144,12 +1146,14 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > case FBIOPAN_DISPLAY: > if (copy_from_user(&var, argp, sizeof(var))) > return -EFAULT; > - if (!lock_fb_info(info)) > - return -ENODEV; > console_lock(); > + if (!lock_fb_info(info)) { > + console_unlock(); > + return -ENODEV; > + } > ret = fb_pan_display(info, &var); > - console_unlock(); > unlock_fb_info(info); > + console_unlock(); > if (ret == 0 && copy_to_user(argp, &var, sizeof(var))) > return -EFAULT; > break; > @@ -1184,23 +1188,27 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > break; > } > event.data = &con2fb; > - if (!lock_fb_info(info)) > - return -ENODEV; > console_lock(); > + if (!lock_fb_info(info)) { > + console_unlock(); > + return -ENODEV; > + } > event.info = info; > ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP, &event); > - console_unlock(); > unlock_fb_info(info); > + console_unlock(); > break; > case FBIOBLANK: > - if (!lock_fb_info(info)) > - return -ENODEV; > console_lock(); > + if (!lock_fb_info(info)) { > + console_unlock(); > + return -ENODEV; > + } > info->flags |= FBINFO_MISC_USEREVENT; > ret = fb_blank(info, arg); > info->flags &= ~FBINFO_MISC_USEREVENT; > - console_unlock(); > unlock_fb_info(info); > + console_unlock(); > break; > default: > if (!lock_fb_info(info)) > @@ -1660,12 +1668,15 @@ static int do_register_framebuffer(struct fb_info *fb_info) > registered_fb[i] = fb_info; > > event.info = fb_info; > - if (!lock_fb_info(fb_info)) > - return -ENODEV; > console_lock(); > + if (!lock_fb_info(fb_info)) { > + console_unlock(); > + return -ENODEV; > + } > + > fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event); > - console_unlock(); > unlock_fb_info(fb_info); > + console_unlock(); > return 0; > } > > @@ -1678,13 +1689,16 @@ static int do_unregister_framebuffer(struct fb_info *fb_info) > if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info) > return -EINVAL; > > - if (!lock_fb_info(fb_info)) > - return -ENODEV; > console_lock(); > + if (!lock_fb_info(fb_info)) { > + console_unlock(); > + return -ENODEV; > + } > + > event.info = fb_info; > ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event); > - console_unlock(); > unlock_fb_info(fb_info); > + console_unlock(); > > if (ret) > return -EINVAL; -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 2013-10-28 08:01, Gu Zheng wrote: > Following commits: > 50e244cc79 fb: rework locking to fix lock ordering on takeover > e93a9a8687 fb: Yet another band-aid for fixing lockdep mess > 054430e773 fbcon: fix locking harder > > reworked locking to fix related lock ordering on takeover, and introduced console_lock > into fbmem, but it seems that the new lock sequence(fb_info->lock ---> console_lock) > is against with the one in console_callback(console_lock ---> fb_info->lock), and leads to > a potential deadlock as following: A quick grep shows that there are other places than fbmem.c which use lock_fb_info and console_lock, for example drivers/video/sh_mobile_lcdcfb.c. Tomi
Hi Tomi, On 10/30/2013 07:20 PM, Tomi Valkeinen wrote: > Hi, > > On 2013-10-28 08:01, Gu Zheng wrote: >> Following commits: >> 50e244cc79 fb: rework locking to fix lock ordering on takeover >> e93a9a8687 fb: Yet another band-aid for fixing lockdep mess >> 054430e773 fbcon: fix locking harder >> >> reworked locking to fix related lock ordering on takeover, and introduced console_lock >> into fbmem, but it seems that the new lock sequence(fb_info->lock ---> console_lock) >> is against with the one in console_callback(console_lock ---> fb_info->lock), and leads to >> a potential deadlock as following: > > A quick grep shows that there are other places than fbmem.c which use > lock_fb_info and console_lock, for example drivers/video/sh_mobile_lcdcfb.c. Yes, thanks for your reminder, I'll fix them all in the next version. Regards, Gu > > Tomi > > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c index dacaf74..010d191 100644 --- a/drivers/video/fbmem.c +++ b/drivers/video/fbmem.c @@ -1108,14 +1108,16 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, case FBIOPUT_VSCREENINFO: if (copy_from_user(&var, argp, sizeof(var))) return -EFAULT; - if (!lock_fb_info(info)) - return -ENODEV; console_lock(); + if (!lock_fb_info(info)) { + console_unlock(); + return -ENODEV; + } info->flags |= FBINFO_MISC_USEREVENT; ret = fb_set_var(info, &var); info->flags &= ~FBINFO_MISC_USEREVENT; - console_unlock(); unlock_fb_info(info); + console_unlock(); if (!ret && copy_to_user(argp, &var, sizeof(var))) ret = -EFAULT; break; @@ -1144,12 +1146,14 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, case FBIOPAN_DISPLAY: if (copy_from_user(&var, argp, sizeof(var))) return -EFAULT; - if (!lock_fb_info(info)) - return -ENODEV; console_lock(); + if (!lock_fb_info(info)) { + console_unlock(); + return -ENODEV; + } ret = fb_pan_display(info, &var); - console_unlock(); unlock_fb_info(info); + console_unlock(); if (ret == 0 && copy_to_user(argp, &var, sizeof(var))) return -EFAULT; break; @@ -1184,23 +1188,27 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, break; } event.data = &con2fb; - if (!lock_fb_info(info)) - return -ENODEV; console_lock(); + if (!lock_fb_info(info)) { + console_unlock(); + return -ENODEV; + } event.info = info; ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP, &event); - console_unlock(); unlock_fb_info(info); + console_unlock(); break; case FBIOBLANK: - if (!lock_fb_info(info)) - return -ENODEV; console_lock(); + if (!lock_fb_info(info)) { + console_unlock(); + return -ENODEV; + } info->flags |= FBINFO_MISC_USEREVENT; ret = fb_blank(info, arg); info->flags &= ~FBINFO_MISC_USEREVENT; - console_unlock(); unlock_fb_info(info); + console_unlock(); break; default: if (!lock_fb_info(info)) @@ -1660,12 +1668,15 @@ static int do_register_framebuffer(struct fb_info *fb_info) registered_fb[i] = fb_info; event.info = fb_info; - if (!lock_fb_info(fb_info)) - return -ENODEV; console_lock(); + if (!lock_fb_info(fb_info)) { + console_unlock(); + return -ENODEV; + } + fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event); - console_unlock(); unlock_fb_info(fb_info); + console_unlock(); return 0; } @@ -1678,13 +1689,16 @@ static int do_unregister_framebuffer(struct fb_info *fb_info) if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info) return -EINVAL; - if (!lock_fb_info(fb_info)) - return -ENODEV; console_lock(); + if (!lock_fb_info(fb_info)) { + console_unlock(); + return -ENODEV; + } + event.info = fb_info; ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event); - console_unlock(); unlock_fb_info(fb_info); + console_unlock(); if (ret) return -EINVAL;
Following commits: 50e244cc79 fb: rework locking to fix lock ordering on takeover e93a9a8687 fb: Yet another band-aid for fixing lockdep mess 054430e773 fbcon: fix locking harder reworked locking to fix related lock ordering on takeover, and introduced console_lock into fbmem, but it seems that the new lock sequence(fb_info->lock ---> console_lock) is against with the one in console_callback(console_lock ---> fb_info->lock), and leads to a potential deadlock as following: [ 601.079000] ====================================================== [ 601.079000] [ INFO: possible circular locking dependency detected ] [ 601.079000] 3.11.0 #189 Not tainted [ 601.079000] ------------------------------------------------------- [ 601.079000] kworker/0:3/619 is trying to acquire lock: [ 601.079000] (&fb_info->lock){+.+.+.}, at: [<ffffffff81397566>] lock_fb_info+0x26/0x60 [ 601.079000] but task is already holding lock: [ 601.079000] (console_lock){+.+.+.}, at: [<ffffffff8141aae3>] console_callback+0x13/0x160 [ 601.079000] which lock already depends on the new lock. [ 601.079000] the existing dependency chain (in reverse order) is: [ 601.079000] -> #1 (console_lock){+.+.+.}: [ 601.079000] [<ffffffff810dc971>] lock_acquire+0xa1/0x140 [ 601.079000] [<ffffffff810c6267>] console_lock+0x77/0x80 [ 601.079000] [<ffffffff81399448>] register_framebuffer+0x1d8/0x320 [ 601.079000] [<ffffffff81cfb4c8>] efifb_probe+0x408/0x48f [ 601.079000] [<ffffffff8144a963>] platform_drv_probe+0x43/0x80 [ 601.079000] [<ffffffff8144853b>] driver_probe_device+0x8b/0x390 [ 601.079000] [<ffffffff814488eb>] __driver_attach+0xab/0xb0 [ 601.079000] [<ffffffff814463bd>] bus_for_each_dev+0x5d/0xa0 [ 601.079000] [<ffffffff81447e6e>] driver_attach+0x1e/0x20 [ 601.079000] [<ffffffff81447a07>] bus_add_driver+0x117/0x290 [ 601.079000] [<ffffffff81448fea>] driver_register+0x7a/0x170 [ 601.079000] [<ffffffff8144a10a>] __platform_driver_register+0x4a/0x50 [ 601.079000] [<ffffffff8144a12d>] platform_driver_probe+0x1d/0xb0 [ 601.079000] [<ffffffff81cfb0a1>] efifb_init+0x273/0x292 [ 601.079000] [<ffffffff81002132>] do_one_initcall+0x102/0x1c0 [ 601.079000] [<ffffffff81cb80a6>] kernel_init_freeable+0x15d/0x1ef [ 601.079000] [<ffffffff8166d2de>] kernel_init+0xe/0xf0 [ 601.079000] [<ffffffff816914ec>] ret_from_fork+0x7c/0xb0 [ 601.079000] -> #0 (&fb_info->lock){+.+.+.}: [ 601.079000] [<ffffffff810dc1d8>] __lock_acquire+0x1e18/0x1f10 [ 601.079000] [<ffffffff810dc971>] lock_acquire+0xa1/0x140 [ 601.079000] [<ffffffff816835ca>] mutex_lock_nested+0x7a/0x3b0 [ 601.079000] [<ffffffff81397566>] lock_fb_info+0x26/0x60 [ 601.079000] [<ffffffff813a4aeb>] fbcon_blank+0x29b/0x2e0 [ 601.079000] [<ffffffff81418658>] do_blank_screen+0x1d8/0x280 [ 601.079000] [<ffffffff8141ab34>] console_callback+0x64/0x160 [ 601.079000] [<ffffffff8108d855>] process_one_work+0x1f5/0x540 [ 601.079000] [<ffffffff8108e04c>] worker_thread+0x11c/0x370 [ 601.079000] [<ffffffff81095fbd>] kthread+0xed/0x100 [ 601.079000] [<ffffffff816914ec>] ret_from_fork+0x7c/0xb0 [ 601.079000] other info that might help us debug this: [ 601.079000] Possible unsafe locking scenario: [ 601.079000] CPU0 CPU1 [ 601.079000] ---- ---- [ 601.079000] lock(console_lock); [ 601.079000] lock(&fb_info->lock); [ 601.079000] lock(console_lock); [ 601.079000] lock(&fb_info->lock); [ 601.079000] *** DEADLOCK *** so we reorder the lock sequence the same as it in console_callback() to avoid this issue. Not very sure this change is suitable, any comments is welcome. Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com> --- drivers/video/fbmem.c | 50 +++++++++++++++++++++++++++++++----------------- 1 files changed, 32 insertions(+), 18 deletions(-)