.29rc2 lockdep report. fb_mmap vs sys_mmap2

.29rc2 lockdep report. fb_mmap vs sys_mmap2

[Dave Jones]

Two mmaps enter! Fight!


 =======================================================
 [ INFO: possible circular locking dependency detected ]
 2.6.29-0.41.rc2.fc11.i686 #1
 -------------------------------------------------------
 plymouthd/669 is trying to acquire lock:
 (&fb_info->lock){--..}, at: [<c055355c>] fb_mmap+0x87/0x156

 but task is already holding lock:
 (&mm->mmap_sem){----}, at: [<c0406a5d>] sys_mmap2+0x44/0x7b

 which lock already depends on the new lock.


 the existing dependency chain (in reverse order) is:

 -> #1 (&mm->mmap_sem){----}:
       [<c044fef3>] __lock_acquire+0x9af/0xb22
       [<c04500c1>] lock_acquire+0x5b/0x81
       [<c048e036>] might_fault+0x60/0x80
       [<c053dedc>] copy_to_user+0x2c/0xfc
       [<c05542cc>] fb_ioctl+0x247/0x338
       [<c04b0c9f>] vfs_ioctl+0x22/0x69
       [<c04b1214>] do_vfs_ioctl+0x46a/0x4a3
       [<c04b128d>] sys_ioctl+0x40/0x5a
       [<c0403b76>] syscall_call+0x7/0xb
       [<ffffffff>] 0xffffffff

 -> #0 (&fb_info->lock){--..}:
       [<c044fdc8>] __lock_acquire+0x884/0xb22
       [<c04500c1>] lock_acquire+0x5b/0x81
       [<c06e1b98>] __mutex_lock_common+0xd5/0x329
       [<c06e1e84>] mutex_lock_nested+0x2e/0x36
       [<c055355c>] fb_mmap+0x87/0x156
       [<c0494620>] mmap_region+0x22b/0x43c
       [<c0494aa2>] do_mmap_pgoff+0x271/0x2d1
       [<c0406a73>] sys_mmap2+0x5a/0x7b
       [<c0403b76>] syscall_call+0x7/0xb
       [<ffffffff>] 0xffffffff

 other info that might help us debug this:

 1 lock held by plymouthd/669:
 #0:  (&mm->mmap_sem){----}, at: [<c0406a5d>] sys_mmap2+0x44/0x7b

 stack backtrace:
 Pid: 669, comm: plymouthd Not tainted 2.6.29-0.41.rc2.fc11.i686 #1
 Call Trace:
 [<c06e09b6>] ? printk+0xf/0x11
 [<c044f32d>] print_circular_bug_tail+0x5d/0x68
 [<c044fdc8>] __lock_acquire+0x884/0xb22
 [<c04500c1>] lock_acquire+0x5b/0x81
 [<c055355c>] ? fb_mmap+0x87/0x156
 [<c06e1b98>] __mutex_lock_common+0xd5/0x329
 [<c055355c>] ? fb_mmap+0x87/0x156
 [<c06e1e84>] mutex_lock_nested+0x2e/0x36
 [<c055355c>] ? fb_mmap+0x87/0x156
 [<c055355c>] fb_mmap+0x87/0x156
 [<c0494620>] mmap_region+0x22b/0x43c
 [<c0494aa2>] do_mmap_pgoff+0x271/0x2d1
 [<c0406a73>] sys_mmap2+0x5a/0x7b
 [<c0403b76>] syscall_call+0x7/0xb


-- 
http://www.codemonkey.org.uk

Re: .29rc2 lockdep report. fb_mmap vs sys_mmap2

[Johannes Weiner]

On Sat, Jan 17, 2009 at 06:19:25PM -0500, Dave Jones wrote:
> Two mmaps enter! Fight!
> 
> 
>  =======================================================
>  [ INFO: possible circular locking dependency detected ]
>  2.6.29-0.41.rc2.fc11.i686 #1
>  -------------------------------------------------------
>  plymouthd/669 is trying to acquire lock:
>  (&fb_info->lock){--..}, at: [<c055355c>] fb_mmap+0x87/0x156
> 
>  but task is already holding lock:
>  (&mm->mmap_sem){----}, at: [<c0406a5d>] sys_mmap2+0x44/0x7b
> 
>  which lock already depends on the new lock.
> 
> 
>  the existing dependency chain (in reverse order) is:
> 
>  -> #1 (&mm->mmap_sem){----}:
>        [<c044fef3>] __lock_acquire+0x9af/0xb22
>        [<c04500c1>] lock_acquire+0x5b/0x81
>        [<c048e036>] might_fault+0x60/0x80
>        [<c053dedc>] copy_to_user+0x2c/0xfc
>        [<c05542cc>] fb_ioctl+0x247/0x338
>        [<c04b0c9f>] vfs_ioctl+0x22/0x69
>        [<c04b1214>] do_vfs_ioctl+0x46a/0x4a3
>        [<c04b128d>] sys_ioctl+0x40/0x5a
>        [<c0403b76>] syscall_call+0x7/0xb
>        [<ffffffff>] 0xffffffff
> 
>  -> #0 (&fb_info->lock){--..}:
>        [<c044fdc8>] __lock_acquire+0x884/0xb22
>        [<c04500c1>] lock_acquire+0x5b/0x81
>        [<c06e1b98>] __mutex_lock_common+0xd5/0x329
>        [<c06e1e84>] mutex_lock_nested+0x2e/0x36
>        [<c055355c>] fb_mmap+0x87/0x156
>        [<c0494620>] mmap_region+0x22b/0x43c
>        [<c0494aa2>] do_mmap_pgoff+0x271/0x2d1
>        [<c0406a73>] sys_mmap2+0x5a/0x7b
>        [<c0403b76>] syscall_call+0x7/0xb
>        [<ffffffff>] 0xffffffff
> 
>  other info that might help us debug this:
> 
>  1 lock held by plymouthd/669:
>  #0:  (&mm->mmap_sem){----}, at: [<c0406a5d>] sys_mmap2+0x44/0x7b
> 
>  stack backtrace:
>  Pid: 669, comm: plymouthd Not tainted 2.6.29-0.41.rc2.fc11.i686 #1
>  Call Trace:
>  [<c06e09b6>] ? printk+0xf/0x11
>  [<c044f32d>] print_circular_bug_tail+0x5d/0x68
>  [<c044fdc8>] __lock_acquire+0x884/0xb22
>  [<c04500c1>] lock_acquire+0x5b/0x81
>  [<c055355c>] ? fb_mmap+0x87/0x156
>  [<c06e1b98>] __mutex_lock_common+0xd5/0x329
>  [<c055355c>] ? fb_mmap+0x87/0x156
>  [<c06e1e84>] mutex_lock_nested+0x2e/0x36
>  [<c055355c>] ? fb_mmap+0x87/0x156
>  [<c055355c>] fb_mmap+0x87/0x156
>  [<c0494620>] mmap_region+0x22b/0x43c
>  [<c0494aa2>] do_mmap_pgoff+0x271/0x2d1
>  [<c0406a73>] sys_mmap2+0x5a/0x7b
>  [<c0403b76>] syscall_call+0x7/0xb

I reported this one already some time ago [1], Andrea Righi [cc'd]
also had a fix [2] but it seemed to not have made it in.  Andrea,
perhaps you can resend it?

	Hannes

[1] http://lkml.org/lkml/2008/10/22/759
[2] http://lkml.org/lkml/2008/10/28/377

Re: .29rc2 lockdep report. fb_mmap vs sys_mmap2

[Andrea Righi]

On 2009-01-18 03:00, Johannes Weiner wrote:
> On Sat, Jan 17, 2009 at 06:19:25PM -0500, Dave Jones wrote:
>> Two mmaps enter! Fight!
>>
>>
>>  =======================================================
>>  [ INFO: possible circular locking dependency detected ]
>>  2.6.29-0.41.rc2.fc11.i686 #1
>>  -------------------------------------------------------
>>  plymouthd/669 is trying to acquire lock:
>>  (&fb_info->lock){--..}, at: [<c055355c>] fb_mmap+0x87/0x156
>>
>>  but task is already holding lock:
>>  (&mm->mmap_sem){----}, at: [<c0406a5d>] sys_mmap2+0x44/0x7b
>>
>>  which lock already depends on the new lock.
>>
>>
>>  the existing dependency chain (in reverse order) is:
>>
>>  -> #1 (&mm->mmap_sem){----}:
>>        [<c044fef3>] __lock_acquire+0x9af/0xb22
>>        [<c04500c1>] lock_acquire+0x5b/0x81
>>        [<c048e036>] might_fault+0x60/0x80
>>        [<c053dedc>] copy_to_user+0x2c/0xfc
>>        [<c05542cc>] fb_ioctl+0x247/0x338
>>        [<c04b0c9f>] vfs_ioctl+0x22/0x69
>>        [<c04b1214>] do_vfs_ioctl+0x46a/0x4a3
>>        [<c04b128d>] sys_ioctl+0x40/0x5a
>>        [<c0403b76>] syscall_call+0x7/0xb
>>        [<ffffffff>] 0xffffffff
>>
>>  -> #0 (&fb_info->lock){--..}:
>>        [<c044fdc8>] __lock_acquire+0x884/0xb22
>>        [<c04500c1>] lock_acquire+0x5b/0x81
>>        [<c06e1b98>] __mutex_lock_common+0xd5/0x329
>>        [<c06e1e84>] mutex_lock_nested+0x2e/0x36
>>        [<c055355c>] fb_mmap+0x87/0x156
>>        [<c0494620>] mmap_region+0x22b/0x43c
>>        [<c0494aa2>] do_mmap_pgoff+0x271/0x2d1
>>        [<c0406a73>] sys_mmap2+0x5a/0x7b
>>        [<c0403b76>] syscall_call+0x7/0xb
>>        [<ffffffff>] 0xffffffff
>>
>>  other info that might help us debug this:
>>
>>  1 lock held by plymouthd/669:
>>  #0:  (&mm->mmap_sem){----}, at: [<c0406a5d>] sys_mmap2+0x44/0x7b
>>
>>  stack backtrace:
>>  Pid: 669, comm: plymouthd Not tainted 2.6.29-0.41.rc2.fc11.i686 #1
>>  Call Trace:
>>  [<c06e09b6>] ? printk+0xf/0x11
>>  [<c044f32d>] print_circular_bug_tail+0x5d/0x68
>>  [<c044fdc8>] __lock_acquire+0x884/0xb22
>>  [<c04500c1>] lock_acquire+0x5b/0x81
>>  [<c055355c>] ? fb_mmap+0x87/0x156
>>  [<c06e1b98>] __mutex_lock_common+0xd5/0x329
>>  [<c055355c>] ? fb_mmap+0x87/0x156
>>  [<c06e1e84>] mutex_lock_nested+0x2e/0x36
>>  [<c055355c>] ? fb_mmap+0x87/0x156
>>  [<c055355c>] fb_mmap+0x87/0x156
>>  [<c0494620>] mmap_region+0x22b/0x43c
>>  [<c0494aa2>] do_mmap_pgoff+0x271/0x2d1
>>  [<c0406a73>] sys_mmap2+0x5a/0x7b
>>  [<c0403b76>] syscall_call+0x7/0xb
> 
> I reported this one already some time ago [1], Andrea Righi [cc'd]
> also had a fix [2] but it seemed to not have made it in.  Andrea,
> perhaps you can resend it?
> 
> 	Hannes
> 
> [1] http://lkml.org/lkml/2008/10/22/759
> [2] http://lkml.org/lkml/2008/10/28/377

Sure, I can resend it soon, after a test.

-Andrea

[PATCH] fbmem: fix copy_from/to_user() with mutex held

[Andrea Righi]

Avoid to call copy_from/to_user() with fb_info->lock mutex held in fbmem
fb_ioctl().

NOTE: it doesn't push down the fb_info->lock in each driver's fb_ioctl().

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
---
 drivers/video/fbcmap.c |   21 +++++--
 drivers/video/fbmem.c  |  158 ++++++++++++++++++++++++++++--------------------
 include/linux/fb.h     |    5 +-
 3 files changed, 112 insertions(+), 72 deletions(-)

diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
index 91b78e6..b19f12c 100644
--- a/drivers/video/fbcmap.c
+++ b/drivers/video/fbcmap.c
@@ -250,10 +250,6 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 	int rc, size = cmap->len * sizeof(u16);
 	struct fb_cmap umap;
 
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-			        !info->fbops->fb_setcmap))
-		return -EINVAL;
-
 	memset(&umap, 0, sizeof(struct fb_cmap));
 	rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);
 	if (rc)
@@ -262,11 +258,24 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 	    copy_from_user(umap.green, cmap->green, size) ||
 	    copy_from_user(umap.blue, cmap->blue, size) ||
 	    (cmap->transp && copy_from_user(umap.transp, cmap->transp, size))) {
-		fb_dealloc_cmap(&umap);
-		return -EFAULT;
+		rc = -EFAULT;
+		goto out;
 	}
 	umap.start = cmap->start;
+	info = get_fb_info(info);
+	if (!info) {
+		rc = -ENODEV;
+		goto out;
+	}
+	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
+				!info->fbops->fb_setcmap)) {
+		rc = -EINVAL;
+		goto out1;
+	}
 	rc = fb_set_cmap(&umap, info);
+out1:
+	put_fb_info(info);
+out:
 	fb_dealloc_cmap(&umap);
 	return rc;
 }
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 756efeb..e51e9ba 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1006,6 +1006,23 @@ fb_blank(struct fb_info *info, int blank)
  	return ret;
 }
 
+struct fb_info *get_fb_info(struct fb_info *info)
+__acquires(&info->lock)
+{
+	mutex_lock(&info->lock);
+	if (!info->fbops) {
+		mutex_unlock(&info->lock);
+		return NULL;
+	}
+	return info;
+}
+
+void put_fb_info(struct fb_info *info)
+__releases(&info->lock)
+{
+	mutex_unlock(&info->lock);
+}
+
 static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			unsigned long arg)
 {
@@ -1013,25 +1030,28 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 	struct fb_var_screeninfo var;
 	struct fb_fix_screeninfo fix;
 	struct fb_con2fbmap con2fb;
+	struct fb_cmap cmap_from;
 	struct fb_cmap_user cmap;
 	struct fb_event event;
 	void __user *argp = (void __user *)arg;
 	long ret = 0;
 
-	fb = info->fbops;
-	if (!fb)
-		return -ENODEV;
-
 	switch (cmd) {
 	case FBIOGET_VSCREENINFO:
-		ret = copy_to_user(argp, &info->var,
-				    sizeof(var)) ? -EFAULT : 0;
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
+		memcpy(&var, &info->var, sizeof(var));
+		put_fb_info(info);
+
+		ret = copy_to_user(argp, &var, sizeof(var)) ? -EFAULT : 0;
 		break;
 	case FBIOPUT_VSCREENINFO:
-		if (copy_from_user(&var, argp, sizeof(var))) {
-			ret =  -EFAULT;
-			break;
-		}
+		if (copy_from_user(&var, argp, sizeof(var)))
+			return -EFAULT;
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
 		acquire_console_sem();
 		info->flags |= FBINFO_MISC_USEREVENT;
 		ret = fb_set_var(info, &var);
@@ -1041,104 +1061,116 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			ret = -EFAULT;
 		break;
 	case FBIOGET_FSCREENINFO:
-		ret = copy_to_user(argp, &info->fix,
-				    sizeof(fix)) ? -EFAULT : 0;
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
+		memcpy(&fix, &info->fix, sizeof(fix));
+		put_fb_info(info);
+
+		ret = copy_to_user(argp, &fix, sizeof(fix)) ? -EFAULT : 0;
 		break;
 	case FBIOPUTCMAP:
 		if (copy_from_user(&cmap, argp, sizeof(cmap)))
-			ret = -EFAULT;
-		else
-			ret = fb_set_user_cmap(&cmap, info);
+			return -EFAULT;
+		ret = fb_set_user_cmap(&cmap, info);
 		break;
 	case FBIOGETCMAP:
 		if (copy_from_user(&cmap, argp, sizeof(cmap)))
-			ret = -EFAULT;
-		else
-			ret = fb_cmap_to_user(&info->cmap, &cmap);
+			return -EFAULT;
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
+		memcpy(&cmap_from, &info->cmap, sizeof(cmap_from));
+		put_fb_info(info);
+		ret = fb_cmap_to_user(&cmap_from, &cmap);
 		break;
 	case FBIOPAN_DISPLAY:
-		if (copy_from_user(&var, argp, sizeof(var))) {
-			ret = -EFAULT;
-			break;
-		}
+		if (copy_from_user(&var, argp, sizeof(var)))
+			return -EFAULT;
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
 		acquire_console_sem();
 		ret = fb_pan_display(info, &var);
 		release_console_sem();
+		put_fb_info(info);
 		if (ret == 0 && copy_to_user(argp, &var, sizeof(var)))
-			ret = -EFAULT;
+			return -EFAULT;
 		break;
 	case FBIO_CURSOR:
 		ret = -EINVAL;
 		break;
 	case FBIOGET_CON2FBMAP:
 		if (copy_from_user(&con2fb, argp, sizeof(con2fb)))
-			ret = -EFAULT;
-		else if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
-			ret = -EINVAL;
-		else {
-			con2fb.framebuffer = -1;
-			event.info = info;
-			event.data = &con2fb;
-			fb_notifier_call_chain(FB_EVENT_GET_CONSOLE_MAP,
-								&event);
-			ret = copy_to_user(argp, &con2fb,
-				    sizeof(con2fb)) ? -EFAULT : 0;
-		}
+			return -EFAULT;
+		if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
+			return -EINVAL;
+		con2fb.framebuffer = -1;
+		event.data = &con2fb;
+
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
+		event.info = info;
+		fb_notifier_call_chain(FB_EVENT_GET_CONSOLE_MAP, &event);
+		put_fb_info(info);
+
+		ret = copy_to_user(argp, &con2fb, sizeof(con2fb)) ? -EFAULT : 0;
 		break;
 	case FBIOPUT_CON2FBMAP:
-		if (copy_from_user(&con2fb, argp, sizeof(con2fb))) {
-			ret = -EFAULT;
-			break;
-		}
-		if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES) {
-			ret = -EINVAL;
-			break;
-		}
-		if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX) {
-			ret = -EINVAL;
-			break;
-		}
+		if (copy_from_user(&con2fb, argp, sizeof(con2fb)))
+			return -EFAULT;
+		if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
+			return -EINVAL;
+		if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
+			return -EINVAL;
 		if (!registered_fb[con2fb.framebuffer])
 			request_module("fb%d", con2fb.framebuffer);
 		if (!registered_fb[con2fb.framebuffer]) {
 			ret = -EINVAL;
 			break;
 		}
-		event.info = info;
 		event.data = &con2fb;
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
+		event.info = info;
 		ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP,
 					      &event);
+		put_fb_info(info);
 		break;
 	case FBIOBLANK:
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
 		acquire_console_sem();
 		info->flags |= FBINFO_MISC_USEREVENT;
 		ret = fb_blank(info, arg);
 		info->flags &= ~FBINFO_MISC_USEREVENT;
 		release_console_sem();
-		break;;
+		put_fb_info(info);
+		break;
 	default:
-		if (fb->fb_ioctl == NULL)
-			ret = -ENOTTY;
-		else
+		info = get_fb_info(info);
+		if (!info)
+			return -ENODEV;
+		fb = info->fbops;
+		if (fb->fb_ioctl)
 			ret = fb->fb_ioctl(info, cmd, arg);
+		else
+			ret = -ENOTTY;
+		put_fb_info(info);
 	}
 	return ret;
 }
 
 static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-__acquires(&info->lock)
-__releases(&info->lock)
 {
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int fbidx = iminor(inode);
-	struct fb_info *info;
-	long ret;
+	struct fb_info *info = registered_fb[fbidx];
 
-	info = registered_fb[fbidx];
-	mutex_lock(&info->lock);
-	ret = do_fb_ioctl(info, cmd, arg);
-	mutex_unlock(&info->lock);
-	return ret;
+	return do_fb_ioctl(info, cmd, arg);
 }
 
 #ifdef CONFIG_COMPAT
@@ -1257,8 +1289,6 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd,
 
 static long fb_compat_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
-__acquires(&info->lock)
-__releases(&info->lock)
 {
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int fbidx = iminor(inode);
@@ -1266,7 +1296,6 @@ __releases(&info->lock)
 	struct fb_ops *fb = info->fbops;
 	long ret = -ENOIOCTLCMD;
 
-	mutex_lock(&info->lock);
 	switch(cmd) {
 	case FBIOGET_VSCREENINFO:
 	case FBIOPUT_VSCREENINFO:
@@ -1292,7 +1321,6 @@ __releases(&info->lock)
 			ret = fb->fb_compat_ioctl(info, cmd, arg);
 		break;
 	}
-	mutex_unlock(&info->lock);
 	return ret;
 }
 #endif
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 818fe21..4f02a63 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -960,6 +960,9 @@ extern struct fb_info *registered_fb[FB_MAX];
 extern int num_registered_fb;
 extern struct class *fb_class;
 
+struct fb_info *get_fb_info(struct fb_info *info);
+void put_fb_info(struct fb_info *info);
+
 static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
 					   u8 *src, u32 s_pitch, u32 height)
 {
@@ -1068,7 +1071,7 @@ extern void fb_dealloc_cmap(struct fb_cmap *cmap);
 extern int fb_copy_cmap(const struct fb_cmap *from, struct fb_cmap *to);
 extern int fb_cmap_to_user(const struct fb_cmap *from, struct fb_cmap_user *to);
 extern int fb_set_cmap(struct fb_cmap *cmap, struct fb_info *fb_info);
-extern int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *fb_info);
+extern int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info);
 extern const struct fb_cmap *fb_default_cmap(int len);
 extern void fb_invert_cmaps(void);
 

Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held

[Johannes Weiner]

Hi,

On Sun, Jan 18, 2009 at 09:17:15PM +0100, Andrea Righi wrote:
> Avoid to call copy_from/to_user() with fb_info->lock mutex held in fbmem
> fb_ioctl().
> 
> NOTE: it doesn't push down the fb_info->lock in each driver's fb_ioctl().
> 
> Signed-off-by: Andrea Righi <righi.andrea@gmail.com>

This should probably also include an explanation for WHY doing uaccess
under fb_info->lock is verboten.  Perhaps even a comment because I
don't think it is obvious from the code.

> ---
>  drivers/video/fbcmap.c |   21 +++++--
>  drivers/video/fbmem.c  |  158 ++++++++++++++++++++++++++++--------------------
>  include/linux/fb.h     |    5 +-
>  3 files changed, 112 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
> index 91b78e6..b19f12c 100644
> --- a/drivers/video/fbcmap.c
> +++ b/drivers/video/fbcmap.c
> @@ -250,10 +250,6 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
>  	int rc, size = cmap->len * sizeof(u16);
>  	struct fb_cmap umap;
>  
> -	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
> -			        !info->fbops->fb_setcmap))
> -		return -EINVAL;
> -
>  	memset(&umap, 0, sizeof(struct fb_cmap));
>  	rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);
>  	if (rc)
> @@ -262,11 +258,24 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
>  	    copy_from_user(umap.green, cmap->green, size) ||
>  	    copy_from_user(umap.blue, cmap->blue, size) ||
>  	    (cmap->transp && copy_from_user(umap.transp, cmap->transp, size))) {
> -		fb_dealloc_cmap(&umap);
> -		return -EFAULT;
> +		rc = -EFAULT;
> +		goto out;
>  	}
>  	umap.start = cmap->start;
> +	info = get_fb_info(info);
> +	if (!info) {
> +		rc = -ENODEV;
> +		goto out;
> +	}
> +	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
> +				!info->fbops->fb_setcmap)) {
> +		rc = -EINVAL;
> +		goto out1;
> +	}
>  	rc = fb_set_cmap(&umap, info);
> +out1:
> +	put_fb_info(info);
> +out:
>  	fb_dealloc_cmap(&umap);
>  	return rc;
>  }
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 756efeb..e51e9ba 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1006,6 +1006,23 @@ fb_blank(struct fb_info *info, int blank)
>   	return ret;
>  }
>  
> +struct fb_info *get_fb_info(struct fb_info *info)
> +__acquires(&info->lock)
> +{
> +	mutex_lock(&info->lock);
> +	if (!info->fbops) {
> +		mutex_unlock(&info->lock);
> +		return NULL;
> +	}
> +	return info;
> +}
> +
> +void put_fb_info(struct fb_info *info)
> +__releases(&info->lock)
> +{
> +	mutex_unlock(&info->lock);
> +}
> +
>  static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>  			unsigned long arg)
>  {
> @@ -1013,25 +1030,28 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>  	struct fb_var_screeninfo var;
>  	struct fb_fix_screeninfo fix;
>  	struct fb_con2fbmap con2fb;
> +	struct fb_cmap cmap_from;
>  	struct fb_cmap_user cmap;
>  	struct fb_event event;
>  	void __user *argp = (void __user *)arg;
>  	long ret = 0;
>  
> -	fb = info->fbops;
> -	if (!fb)
> -		return -ENODEV;
> -
>  	switch (cmd) {
>  	case FBIOGET_VSCREENINFO:
> -		ret = copy_to_user(argp, &info->var,
> -				    sizeof(var)) ? -EFAULT : 0;
> +		info = get_fb_info(info);
> +		if (!info)
> +			return -ENODEV;
> +		memcpy(&var, &info->var, sizeof(var));

You don't need these memcpy()s:

		var = info->var

does the same much more readable.

Thanks,
	hannes

Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held

[Stefan Richter]

Andrea Righi wrote:
> +struct fb_info *get_fb_info(struct fb_info *info)
> +__acquires(&info->lock)
> +{
> +	mutex_lock(&info->lock);
> +	if (!info->fbops) {
> +		mutex_unlock(&info->lock);
> +		return NULL;
> +	}
> +	return info;
> +}
> +
> +void put_fb_info(struct fb_info *info)
> +__releases(&info->lock)
> +{
> +	mutex_unlock(&info->lock);
> +}
> +

These are IMO bad function names.  get_... and put_... sound too much
like functions that either read or write data, or increment and
decrement the reference count of data.  These names don't reflect that
the sole purpose of these functions is to lock and unlock a mutex (i.e.
to serialize some critical sections).

*If* you really need to hide mutex_(un)lock(&info->lock) inside
wrappers, then the names of these wrappers should clearly state what
they do, so that everybody knows immediately what is going on at the
call sites.
-- 
Stefan Richter
-=====-==--= ---= =--==
http://arcgraph.de/sr/

Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held

[Stefan Richter]

Stefan Richter wrote:
> Andrea Righi wrote:
>> +struct fb_info *get_fb_info(struct fb_info *info)
>> +__acquires(&info->lock)
>> +{
>> +	mutex_lock(&info->lock);
>> +	if (!info->fbops) {
>> +		mutex_unlock(&info->lock);
>> +		return NULL;
>> +	}
>> +	return info;
>> +}
>> +
>> +void put_fb_info(struct fb_info *info)
>> +__releases(&info->lock)
>> +{
>> +	mutex_unlock(&info->lock);
>> +}
>> +
> 
> These are IMO bad function names.

PS:  The return value of the mutex_lock wrapper is also bad.  A bool or
int would IMO be clearer, similar to the return value of mutex_trylock.
-- 
Stefan Richter
-=====-==--= ---= =--==
http://arcgraph.de/sr/

Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held

[Harvey Harrison]

On Mon, 2009-01-19 at 09:05 +0100, Stefan Richter wrote:
> Stefan Richter wrote:
> > Andrea Righi wrote:
> >> +struct fb_info *get_fb_info(struct fb_info *info)
> >> +__acquires(&info->lock)
> >> +{
> >> +	mutex_lock(&info->lock);
> >> +	if (!info->fbops) {
> >> +		mutex_unlock(&info->lock);
> >> +		return NULL;
> >> +	}
> >> +	return info;
> >> +}
> >> +
> >> +void put_fb_info(struct fb_info *info)
> >> +__releases(&info->lock)
> >> +{
> >> +	mutex_unlock(&info->lock);
> >> +}
> >> +
> > 
> > These are IMO bad function names.
> 
> PS:  The return value of the mutex_lock wrapper is also bad.  A bool or
> int would IMO be clearer, similar to the return value of mutex_trylock.

That, and there is no possible way to get the sparse annotations right
for that function, which means you'll get no help from sparse in lock
checking.

So I'd suggest just opencoding these where needed instead of the
wrappers.

Harvey

Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held

[Andrea Righi]

On Mon, Jan 19, 2009 at 9:10 AM, Harvey Harrison
<harvey.harrison@gmail.com> wrote:
> On Mon, 2009-01-19 at 09:05 +0100, Stefan Richter wrote:
>> Stefan Richter wrote:
>> > Andrea Righi wrote:
>> >> +struct fb_info *get_fb_info(struct fb_info *info)
>> >> +__acquires(&info->lock)
>> >> +{
>> >> +  mutex_lock(&info->lock);
>> >> +  if (!info->fbops) {
>> >> +          mutex_unlock(&info->lock);
>> >> +          return NULL;
>> >> +  }
>> >> +  return info;
>> >> +}
>> >> +
>> >> +void put_fb_info(struct fb_info *info)
>> >> +__releases(&info->lock)
>> >> +{
>> >> +  mutex_unlock(&info->lock);
>> >> +}
>> >> +
>> >
>> > These are IMO bad function names.
>>
>> PS:  The return value of the mutex_lock wrapper is also bad.  A bool or
>> int would IMO be clearer, similar to the return value of mutex_trylock.
>
> That, and there is no possible way to get the sparse annotations right
> for that function, which means you'll get no help from sparse in lock
> checking.
>
> So I'd suggest just opencoding these where needed instead of the
> wrappers.
>
> Harvey
>

OK, thanks for all the suggestions. I'll fix everything and repost a
new patch ASAP.

-Andrea