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

[Andrey Borzenkov]

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:

[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))

Attachment: signature.asc
Description: This is a digitally signed message part.