lkml.org 
[lkml]   [2013]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH RESEND] fb: reorder the lock sequence to fix a potential lockdep
    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;




    \
     
     \ /
      Last update: 2013-10-30 04:41    [W:3.114 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site