Re: [PATCH -mmotm] fbmem: fix fb_info->lock and mm->mmap_sem circular locking dependency

From: Rafael J. Wysocki
Date: Sun Mar 22 2009 - 13:57:43 EST

Next message: Frederic Weisbecker: "Re: [PATCH 2/4] tracing: add ring_buffer_event_discard() to ringbuffer"
Previous message: Ingo Molnar: "Re: [PATCH 1/4] tracing: add run-time field descriptions for eventfiltering"
In reply to: Andrey Borzenkov: "Re: [PATCH -mmotm] fbmem: fix fb_info->lock and mm->mmap_sem circular locking dependency"
Next in thread: Andrew Morton: "Re: [PATCH -mmotm] fbmem: fix fb_info->lock and mm->mmap_semcircular locking dependency"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sunday 22 March 2009, Andrey Borzenkov wrote:
> On 5 ÑÐÐÑÐÐÑ 2009 00:46:23 Andrea Righi wrote:
> > Fix this circular locking dependencies in the frame buffer console
> > driver pushing down the mutex fb_info->lock.
> >
> > Circular locking dependecies occur calling the blocking
> > fb_notifier_call_chain() with fb_info->lock held. Notifier callbacks
> > can try to acquire mm->mmap_sem, while fb_mmap() acquires the locks
> > in the reverse order mm->mmap_sem => fb_info->lock.
> >
> > Tested-by: Andrey Borzenkov <arvidjaar@xxxxxxx>
> > Signed-off-by: Andrea Righi <righi.andrea@xxxxxxxxx>
>
> Hmm ... is it in current Linus tree? Because I happened to have this again
> in rc8:

It doesn't seems so, at least I don't see it in there.

Thanks,
Rafael

> [47800.440873] =======================================================
> [47800.440950] [ INFO: possible circular locking dependency detected ]
> [47800.440981] 2.6.29-rc8-1avb #35
> [47800.440999] -------------------------------------------------------
> [47800.441026] s2disk/19740 is trying to acquire lock:
> [47800.441057] (&fb_info->lock){--..}, at: [<c02175b7>] fb_mmap+0x97/0x170
> [47800.441149]
> [47800.441152] but task is already holding lock:
> [47800.441183] (&mm->mmap_sem){----}, at: [<c0106e2e>] sys_mmap2+0x8e/0xc0
> [47800.441260]
> [47800.441262] which lock already depends on the new lock.
> [47800.441266]
> [47800.441307]
> [47800.441309] the existing dependency chain (in reverse order) is:
> [47800.441345]
> [47800.441347] -> #3 (&mm->mmap_sem){----}:
> [47800.441426] [<c014347f>] __lock_acquire+0x129f/0x1930
> [47800.441466] [<c0143b6c>] lock_acquire+0x5c/0x80
> [47800.441500] [<c0175f97>] might_fault+0x77/0xa0
> [47800.441548] [<c01fd746>] copy_to_user+0x36/0x120
> [47800.441591] [<c0199eb7>] filldir+0x97/0xe0
> [47800.441627] [<c01d2279>] sysfs_readdir+0x129/0x220
> [47800.441668] [<c019a066>] vfs_readdir+0x86/0xa0
> [47800.441701] [<c019a1a8>] sys_getdents+0x68/0xc0
> [47800.441734] [<c010340a>] syscall_call+0x7/0xb
> [47800.441770] [<ffffffff>] 0xffffffff
> [47800.441839]
> [47800.441842] -> #2 (sysfs_mutex){--..}:
> [47800.441906] [<c014347f>] __lock_acquire+0x129f/0x1930
> [47800.441941] [<c0143b6c>] lock_acquire+0x5c/0x80
> [47800.441974] [<c03556ba>] mutex_lock_nested+0xba/0x2f0
> [47800.442012] [<c01d260c>] sysfs_addrm_start+0x2c/0xc0
> [47800.442012] [<c01d2bc0>] create_dir+0x40/0x90
> [47800.442012] [<c01d2c3b>] sysfs_create_dir+0x2b/0x50
> [47800.442012] [<c01f74ec>] kobject_add_internal+0xbc/0x1b0
> [47800.442012] [<c01f76b1>] kobject_add_varg+0x31/0x50
> [47800.442012] [<c01f772c>] kobject_add+0x2c/0x60
> [47800.442012] [<c026c598>] device_add+0xa8/0x550
> [47800.442012] [<c026ca52>] device_register+0x12/0x20
> [47800.442012] [<c026cb0b>] device_create_vargs+0xab/0xc0
> [47800.442012] [<c026cb48>] device_create+0x28/0x30
> [47800.442012] [<c0265dfd>] register_con_driver+0xed/0x130
> [47800.442012] [<c02672eb>] take_over_console+0x1b/0x50
> [47800.442012] [<c02244fd>] fbcon_takeover+0x5d/0xb0
> [47800.442012] [<c0225170>] fbcon_event_notify+0x820/0x900
> [47800.442012] [<c0137183>] notifier_call_chain+0x53/0xa0
> [47800.442012] [<c0137444>] __blocking_notifier_call_chain+0x44/0x60
> [47800.442012] [<c013747a>] blocking_notifier_call_chain+0x1a/0x20
> [47800.442012] [<c0216f51>] fb_notifier_call_chain+0x11/0x20
> [47800.442012] [<c0217ed8>] register_framebuffer+0x168/0x220
> [47800.442012] [<c047e2f2>] vesafb_probe+0x542/0x783
> [47800.442012] [<c026f72f>] platform_drv_probe+0xf/0x20
> [47800.442012] [<c026e917>] driver_probe_device+0x87/0x1b0
> [47800.442012] [<c026ead8>] __device_attach+0x8/0x10
> [47800.442012] [<c026df5b>] bus_for_each_drv+0x5b/0x80
> [47800.442012] [<c026eb86>] device_attach+0x76/0x80
> [47800.442012] [<c026dd77>] bus_attach_device+0x47/0x70
> [47800.442012] [<c026c813>] device_add+0x323/0x550
> [47800.442012] [<c0270105>] platform_device_add+0x175/0x1c0
> [47800.442012] [<c047e5cd>] vesafb_init+0x9a/0x1ec
> [47800.442012] [<c010111a>] do_one_initcall+0x2a/0x160
> [47800.442012] [<c04694d5>] kernel_init+0x83/0xd5
> [47800.442012] [<c0103a37>] kernel_thread_helper+0x7/0x10
> [47800.442012] [<ffffffff>] 0xffffffff
> [47800.442012]
> [47800.442012] -> #1 ((fb_notifier_list).rwsem){----}:
> [47800.442012] [<c014347f>] __lock_acquire+0x129f/0x1930
> [47800.442012] [<c0143b6c>] lock_acquire+0x5c/0x80
> [47800.442012] [<c0355fb9>] down_read+0x49/0x90
> [47800.442012] [<c013742a>] __blocking_notifier_call_chain+0x2a/0x60
> [47800.442012] [<c013747a>] blocking_notifier_call_chain+0x1a/0x20
> [47800.442012] [<c0216f51>] fb_notifier_call_chain+0x11/0x20
> [47800.442012] [<c021856e>] do_fb_ioctl+0x2ae/0x5d0
> [47800.442012] [<c02188ad>] fb_ioctl+0x1d/0x20
> [47800.442012] [<c0199270>] vfs_ioctl+0x20/0x80
> [47800.442012] [<c0199482>] do_vfs_ioctl+0x72/0x570
> [47800.442012] [<c01999b9>] sys_ioctl+0x39/0x70
> [47800.442012] [<c0103331>] sysenter_do_call+0x12/0x31
> [47800.442012] [<ffffffff>] 0xffffffff
> [47800.442012]
> [47800.442012] -> #0 (&fb_info->lock){--..}:
> [47800.442012] [<c01435ec>] __lock_acquire+0x140c/0x1930
> [47800.442012] [<c0143b6c>] lock_acquire+0x5c/0x80
> [47800.442012] [<c03556ba>] mutex_lock_nested+0xba/0x2f0
> [47800.442012] [<c02175b7>] fb_mmap+0x97/0x170
> [47800.442012] [<c017cbc0>] mmap_region+0x2e0/0x440
> [47800.442012] [<c017ceea>] do_mmap_pgoff+0x1ca/0x2f0
> [47800.442012] [<c0106e4d>] sys_mmap2+0xad/0xc0
> [47800.442012] [<c0103331>] sysenter_do_call+0x12/0x31
> [47800.442012] [<ffffffff>] 0xffffffff
> [47800.442012]
> [47800.442012] other info that might help us debug this:
> [47800.442012]
> [47800.442012] 1 lock held by s2disk/19740:
> [47800.442012] #0: (&mm->mmap_sem){----}, at: [<c0106e2e>]
> sys_mmap2+0x8e/0xc0
> [47800.442012]
> [47800.442012] stack backtrace:
> [47800.442012] Pid: 19740, comm: s2disk Not tainted 2.6.29-rc8-1avb #35
> [47800.442012] Call Trace:
> [47800.442012] [<c0354324>] ? printk+0x18/0x1c
> [47800.442012] [<c0141d8f>] print_circular_bug_tail+0xcf/0xe0
> [47800.442012] [<c01435ec>] __lock_acquire+0x140c/0x1930
> [47800.442012] [<c0132936>] ? add_wait_queue+0x36/0x50
> [47800.442012] [<c013f375>] ? lock_release_holdtime+0x35/0x210
> [47800.442012] [<c0143b6c>] lock_acquire+0x5c/0x80
> [47800.442012] [<c02175b7>] ? fb_mmap+0x97/0x170
> [47800.442012] [<c03556ba>] mutex_lock_nested+0xba/0x2f0
> [47800.442012] [<c02175b7>] ? fb_mmap+0x97/0x170
> [47800.442012] [<c02175b7>] ? fb_mmap+0x97/0x170
> [47800.442012] [<c01892c5>] ? kmem_cache_alloc+0xa5/0x100
> [47800.442012] [<c02175b7>] fb_mmap+0x97/0x170
> [47800.442012] [<c017cbc0>] mmap_region+0x2e0/0x440
> [47800.442012] [<c017ceea>] do_mmap_pgoff+0x1ca/0x2f0
> [47800.442012] [<c0106e4d>] sys_mmap2+0xad/0xc0
> [47800.442012] [<c0103331>] sysenter_do_call+0x12/0x31
> [47803.041426] Syncing filesystems ... done.
> [47803.055587] Freezing user space processes ... (elapsed 0.00 seconds)
> done.
> [47803.059328] Freezing remaining freezable tasks ... (elapsed 0.00 seconds)
> done.
>
>
> > ---
> > drivers/video/backlight/backlight.c | 3 +
> > drivers/video/backlight/lcd.c | 3 +
> > drivers/video/console/fbcon.c | 73
> > ++++++++++++++++++++++++++++++----- drivers/video/fbmem.c
> > | 11 +-----
> > 4 files changed, 70 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/video/backlight/backlight.c
> > b/drivers/video/backlight/backlight.c index 157057c..dd37cbc 100644
> > --- a/drivers/video/backlight/backlight.c
> > +++ b/drivers/video/backlight/backlight.c
> > @@ -35,6 +35,8 @@ static int fb_notifier_callback(struct
> > notifier_block *self, return 0;
> >
> > bd = container_of(self, struct backlight_device, fb_notif);
> > + if (!lock_fb_info(evdata->info))
> > + return -ENODEV;
> > mutex_lock(&bd->ops_lock);
> > if (bd->ops)
> > if (!bd->ops->check_fb ||
> > @@ -47,6 +49,7 @@ static int fb_notifier_callback(struct
> > notifier_block *self, backlight_update_status(bd);
> > }
> > mutex_unlock(&bd->ops_lock);
> > + unlock_fb_info(evdata->info);
> > return 0;
> > }
> >
> > diff --git a/drivers/video/backlight/lcd.c
> > b/drivers/video/backlight/lcd.c index b644947..0bb13df 100644
> > --- a/drivers/video/backlight/lcd.c
> > +++ b/drivers/video/backlight/lcd.c
> > @@ -40,6 +40,8 @@ static int fb_notifier_callback(struct
> > notifier_block *self, if (!ld->ops)
> > return 0;
> >
> > + if (!lock_fb_info(evdata->info))
> > + return -ENODEV;
> > mutex_lock(&ld->ops_lock);
> > if (!ld->ops->check_fb || ld->ops->check_fb(ld, evdata->info)) {
> > if (event == FB_EVENT_BLANK) {
> > @@ -51,6 +53,7 @@ static int fb_notifier_callback(struct
> > notifier_block *self, }
> > }
> > mutex_unlock(&ld->ops_lock);
> > + unlock_fb_info(evdata->info);
> > return 0;
> > }
> >
> > diff --git a/drivers/video/console/fbcon.c
> > b/drivers/video/console/fbcon.c index 1657b96..2cd500a 100644
> > --- a/drivers/video/console/fbcon.c
> > +++ b/drivers/video/console/fbcon.c
> > @@ -2954,8 +2954,11 @@ static int fbcon_fb_unbind(int idx)
> >
> > static int fbcon_fb_unregistered(struct fb_info *info)
> > {
> > - int i, idx = info->node;
> > + int i, idx;
> >
> > + if (!lock_fb_info(info))
> > + return -ENODEV;
> > + idx = info->node;
> > for (i = first_fb_vc; i <= last_fb_vc; i++) {
> > if (con2fb_map[i] == idx)
> > con2fb_map[i] = -1;
> > @@ -2979,13 +2982,14 @@ static int fbcon_fb_unregistered(struct
> > fb_info *info) }
> > }
> >
> > - if (!num_registered_fb)
> > - unregister_con_driver(&fb_con);
> > -
> > -
> > if (primary_device == idx)
> > primary_device = -1;
> >
> > + unlock_fb_info(info);
> > +
> > + if (!num_registered_fb)
> > + unregister_con_driver(&fb_con);
> > +
> > return 0;
> > }
> >
> > @@ -3021,9 +3025,13 @@ static inline void fbcon_select_primary(struct
> > fb_info *info)
> >
> > static int fbcon_fb_registered(struct fb_info *info)
> > {
> > - int ret = 0, i, idx = info->node;
> > + int ret = 0, i, idx;
> >
> > + if (!lock_fb_info(info))
> > + return -ENODEV;
> > + idx = info->node;
> > fbcon_select_primary(info);
> > + unlock_fb_info(info);
> >
> > if (info_idx == -1) {
> > for (i = first_fb_vc; i <= last_fb_vc; i++) {
> > @@ -3124,7 +3132,7 @@ static void fbcon_get_requirement(struct
> > fb_info *info, }
> > }
> >
> > -static int fbcon_event_notify(struct notifier_block *self,
> > +static int fbcon_event_notify(struct notifier_block *self,
> > unsigned long action, void *data)
> > {
> > struct fb_event *event = data;
> > @@ -3132,7 +3140,7 @@ static int fbcon_event_notify(struct
> > notifier_block *self, struct fb_videomode *mode;
> > struct fb_con2fbmap *con2fb;
> > struct fb_blit_caps *caps;
> > - int ret = 0;
> > + int idx, ret = 0;
> >
> > /*
> > * ignore all events except driver registration and deregistration
> > @@ -3144,23 +3152,54 @@ static int fbcon_event_notify(struct
> > notifier_block *self,
> >
> > switch(action) {
> > case FB_EVENT_SUSPEND:
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > fbcon_suspended(info);
> > + unlock_fb_info(info);
> > break;
> > case FB_EVENT_RESUME:
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > fbcon_resumed(info);
> > + unlock_fb_info(info);
> > break;
> > case FB_EVENT_MODE_CHANGE:
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > fbcon_modechanged(info);
> > + unlock_fb_info(info);
> > break;
> > case FB_EVENT_MODE_CHANGE_ALL:
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > fbcon_set_all_vcs(info);
> > + unlock_fb_info(info);
> > break;
> > case FB_EVENT_MODE_DELETE:
> > mode = event->data;
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > ret = fbcon_mode_deleted(info, mode);
> > + unlock_fb_info(info);
> > break;
> > case FB_EVENT_FB_UNBIND:
> > - ret = fbcon_fb_unbind(info->node);
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > + idx = info->node;
> > + unlock_fb_info(info);
> > + ret = fbcon_fb_unbind(idx);
> > break;
> > case FB_EVENT_FB_REGISTERED:
> > ret = fbcon_fb_registered(info);
> > @@ -3178,17 +3217,31 @@ static int fbcon_event_notify(struct
> > notifier_block *self, con2fb->framebuffer =
> > con2fb_map[con2fb->console - 1];
> > break;
> > case FB_EVENT_BLANK:
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > fbcon_fb_blanked(info, *(int *)event->data);
> > + unlock_fb_info(info);
> > break;
> > case FB_EVENT_NEW_MODELIST:
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > fbcon_new_modelist(info);
> > + unlock_fb_info(info);
> > break;
> > case FB_EVENT_GET_REQ:
> > caps = event->data;
> > + if (!lock_fb_info(info)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > fbcon_get_requirement(info, caps);
> > + unlock_fb_info(info);
> > break;
> > }
> > -
> > done:
> > return ret;
> > }
> > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> > index cfd9dce..b64f061 100644
> > --- a/drivers/video/fbmem.c
> > +++ b/drivers/video/fbmem.c
> > @@ -1086,13 +1086,8 @@ static long do_fb_ioctl(struct fb_info *info,
> > unsigned int cmd, return -EINVAL;
> > con2fb.framebuffer = -1;
> > event.data = &con2fb;
> > -
> > - if (!lock_fb_info(info))
> > - return -ENODEV;
> > event.info = info;
> > fb_notifier_call_chain(FB_EVENT_GET_CONSOLE_MAP, &event);
> > - unlock_fb_info(info);
> > -
> > ret = copy_to_user(argp, &con2fb, sizeof(con2fb)) ? -EFAULT : 0;
> > break;
> > case FBIOPUT_CON2FBMAP:
> > @@ -1109,12 +1104,8 @@ static long do_fb_ioctl(struct fb_info *info,
> > unsigned int cmd, break;
> > }
> > event.data = &con2fb;
> > - if (!lock_fb_info(info))
> > - return -ENODEV;
> > event.info = info;
> > - ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP,
> > - &event);
> > - unlock_fb_info(info);
> > + ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP, &event);
> > break;
> > case FBIOBLANK:
> > if (!lock_fb_info(info))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Frederic Weisbecker: "Re: [PATCH 2/4] tracing: add ring_buffer_event_discard() to ringbuffer"
Previous message: Ingo Molnar: "Re: [PATCH 1/4] tracing: add run-time field descriptions for eventfiltering"
In reply to: Andrey Borzenkov: "Re: [PATCH -mmotm] fbmem: fix fb_info->lock and mm->mmap_sem circular locking dependency"
Next in thread: Andrew Morton: "Re: [PATCH -mmotm] fbmem: fix fb_info->lock and mm->mmap_semcircular locking dependency"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]