Skip to content

Commit d43bdf5

Browse files
Gu Zhengrichard-hu
Gu Zheng
authored and
richard-hu
committed
fb: reorder the lock sequence to fix potential dead lock
Following commits: 50e244c fb: rework locking to fix lock ordering on takeover e93a9a8 fb: Yet another band-aid for fixing lockdep mess 054430e 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 dead lock as following: [ 601.079000] ====================================================== [ 601.079000] [ INFO: possible circular locking dependency detected ] [ 601.079000] 3.11.0 torvalds#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] -> TechNexion#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. And following Tomi's suggestion, fix these similar issues all in fb subsystem. Signed-off-by: Gu Zheng <[email protected]> Signed-off-by: Tomi Valkeinen <[email protected]> (cherry picked from commit fdb31faae11ace02e63e84b39b7840032aa0f1f9)
1 parent f8dce87 commit d43bdf5

File tree

3 files changed

+51
-28
lines changed

3 files changed

+51
-28
lines changed

‎drivers/video/fbmem.c

+32-18
Original file line numberDiff line numberDiff line change
@@ -1099,14 +1099,16 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
10991099
case FBIOPUT_VSCREENINFO:
11001100
if (copy_from_user(&var, argp, sizeof(var)))
11011101
return -EFAULT;
1102-
if (!lock_fb_info(info))
1103-
return -ENODEV;
11041102
console_lock();
1103+
if (!lock_fb_info(info)) {
1104+
console_unlock();
1105+
return -ENODEV;
1106+
}
11051107
info->flags |= FBINFO_MISC_USEREVENT;
11061108
ret = fb_set_var(info, &var);
11071109
info->flags &= ~FBINFO_MISC_USEREVENT;
1108-
console_unlock();
11091110
unlock_fb_info(info);
1111+
console_unlock();
11101112
if (!ret && copy_to_user(argp, &var, sizeof(var)))
11111113
ret = -EFAULT;
11121114
break;
@@ -1135,12 +1137,14 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
11351137
case FBIOPAN_DISPLAY:
11361138
if (copy_from_user(&var, argp, sizeof(var)))
11371139
return -EFAULT;
1138-
if (!lock_fb_info(info))
1139-
return -ENODEV;
11401140
console_lock();
1141+
if (!lock_fb_info(info)) {
1142+
console_unlock();
1143+
return -ENODEV;
1144+
}
11411145
ret = fb_pan_display(info, &var);
1142-
console_unlock();
11431146
unlock_fb_info(info);
1147+
console_unlock();
11441148
if (ret == 0 && copy_to_user(argp, &var, sizeof(var)))
11451149
return -EFAULT;
11461150
break;
@@ -1175,23 +1179,27 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
11751179
break;
11761180
}
11771181
event.data = &con2fb;
1178-
if (!lock_fb_info(info))
1179-
return -ENODEV;
11801182
console_lock();
1183+
if (!lock_fb_info(info)) {
1184+
console_unlock();
1185+
return -ENODEV;
1186+
}
11811187
event.info = info;
11821188
ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP, &event);
1183-
console_unlock();
11841189
unlock_fb_info(info);
1190+
console_unlock();
11851191
break;
11861192
case FBIOBLANK:
1187-
if (!lock_fb_info(info))
1188-
return -ENODEV;
11891193
console_lock();
1194+
if (!lock_fb_info(info)) {
1195+
console_unlock();
1196+
return -ENODEV;
1197+
}
11901198
info->flags |= FBINFO_MISC_USEREVENT;
11911199
ret = fb_blank(info, arg);
11921200
info->flags &= ~FBINFO_MISC_USEREVENT;
1193-
console_unlock();
11941201
unlock_fb_info(info);
1202+
console_unlock();
11951203
break;
11961204
default:
11971205
if (!lock_fb_info(info))
@@ -1649,12 +1657,15 @@ static int do_register_framebuffer(struct fb_info *fb_info)
16491657
registered_fb[i] = fb_info;
16501658

16511659
event.info = fb_info;
1652-
if (!lock_fb_info(fb_info))
1653-
return -ENODEV;
16541660
console_lock();
1661+
if (!lock_fb_info(fb_info)) {
1662+
console_unlock();
1663+
return -ENODEV;
1664+
}
1665+
16551666
fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
1656-
console_unlock();
16571667
unlock_fb_info(fb_info);
1668+
console_unlock();
16581669
return 0;
16591670
}
16601671

@@ -1667,13 +1678,16 @@ static int do_unregister_framebuffer(struct fb_info *fb_info)
16671678
if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
16681679
return -EINVAL;
16691680

1670-
if (!lock_fb_info(fb_info))
1671-
return -ENODEV;
16721681
console_lock();
1682+
if (!lock_fb_info(fb_info)) {
1683+
console_unlock();
1684+
return -ENODEV;
1685+
}
1686+
16731687
event.info = fb_info;
16741688
ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
1675-
console_unlock();
16761689
unlock_fb_info(fb_info);
1690+
console_unlock();
16771691

16781692
if (ret)
16791693
return -EINVAL;

‎drivers/video/fbsysfs.c

+13-6
Original file line numberDiff line numberDiff line change
@@ -177,9 +177,12 @@ static ssize_t store_modes(struct device *device,
177177
if (i * sizeof(struct fb_videomode) != count)
178178
return -EINVAL;
179179

180-
if (!lock_fb_info(fb_info))
181-
return -ENODEV;
182180
console_lock();
181+
if (!lock_fb_info(fb_info)) {
182+
console_unlock();
183+
return -ENODEV;
184+
}
185+
183186
list_splice(&fb_info->modelist, &old_list);
184187
fb_videomode_to_modelist((const struct fb_videomode *)buf, i,
185188
&fb_info->modelist);
@@ -189,8 +192,8 @@ static ssize_t store_modes(struct device *device,
189192
} else
190193
fb_destroy_modelist(&old_list);
191194

192-
console_unlock();
193195
unlock_fb_info(fb_info);
196+
console_unlock();
194197

195198
return 0;
196199
}
@@ -404,12 +407,16 @@ static ssize_t store_fbstate(struct device *device,
404407

405408
state = simple_strtoul(buf, &last, 0);
406409

407-
if (!lock_fb_info(fb_info))
408-
return -ENODEV;
409410
console_lock();
411+
if (!lock_fb_info(fb_info)) {
412+
console_unlock();
413+
return -ENODEV;
414+
}
415+
410416
fb_set_suspend(fb_info, (int)state);
411-
console_unlock();
417+
412418
unlock_fb_info(fb_info);
419+
console_unlock();
413420

414421
return count;
415422
}

‎drivers/video/sh_mobile_lcdcfb.c

+6-4
Original file line numberDiff line numberDiff line change
@@ -574,8 +574,9 @@ static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
574574
switch (event) {
575575
case SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT:
576576
/* HDMI plug in */
577+
console_lock();
577578
if (lock_fb_info(info)) {
578-
console_lock();
579+
579580

580581
ch->display.width = monspec->max_x * 10;
581582
ch->display.height = monspec->max_y * 10;
@@ -594,19 +595,20 @@ static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
594595
fb_set_suspend(info, 0);
595596
}
596597

597-
console_unlock();
598+
598599
unlock_fb_info(info);
599600
}
601+
console_unlock();
600602
break;
601603

602604
case SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT:
603605
/* HDMI disconnect */
606+
console_lock();
604607
if (lock_fb_info(info)) {
605-
console_lock();
606608
fb_set_suspend(info, 1);
607-
console_unlock();
608609
unlock_fb_info(info);
609610
}
611+
console_unlock();
610612
break;
611613

612614
case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:

0 commit comments

Comments
 (0)