* .29rc2 lockdep report. fb_mmap vs sys_mmap2
@ 2009-01-17 23:19 Dave Jones
2009-01-18 2:00 ` Johannes Weiner
0 siblings, 1 reply; 9+ messages in thread
From: Dave Jones @ 2009-01-17 23:19 UTC (permalink / raw)
To: Linux Kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: .29rc2 lockdep report. fb_mmap vs sys_mmap2 2009-01-17 23:19 .29rc2 lockdep report. fb_mmap vs sys_mmap2 Dave Jones @ 2009-01-18 2:00 ` Johannes Weiner 2009-01-18 19:21 ` Andrea Righi 2009-01-18 20:17 ` [PATCH] fbmem: fix copy_from/to_user() with mutex held Andrea Righi 0 siblings, 2 replies; 9+ messages in thread From: Johannes Weiner @ 2009-01-18 2:00 UTC (permalink / raw) To: Dave Jones; +Cc: Linux Kernel, Andrea Righi 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: .29rc2 lockdep report. fb_mmap vs sys_mmap2 2009-01-18 2:00 ` Johannes Weiner @ 2009-01-18 19:21 ` Andrea Righi 2009-01-18 20:17 ` [PATCH] fbmem: fix copy_from/to_user() with mutex held Andrea Righi 1 sibling, 0 replies; 9+ messages in thread From: Andrea Righi @ 2009-01-18 19:21 UTC (permalink / raw) To: Johannes Weiner; +Cc: Dave Jones, Linux Kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] fbmem: fix copy_from/to_user() with mutex held 2009-01-18 2:00 ` Johannes Weiner 2009-01-18 19:21 ` Andrea Righi @ 2009-01-18 20:17 ` Andrea Righi 2009-01-19 7:54 ` Johannes Weiner 2009-01-19 7:58 ` Stefan Richter 1 sibling, 2 replies; 9+ messages in thread From: Andrea Righi @ 2009-01-18 20:17 UTC (permalink / raw) To: Johannes Weiner, Dave Jones Cc: Johannes Weiner, Krzysztof Helt, Andrew Morton, LKML 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); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held 2009-01-18 20:17 ` [PATCH] fbmem: fix copy_from/to_user() with mutex held Andrea Righi @ 2009-01-19 7:54 ` Johannes Weiner 2009-01-19 7:58 ` Stefan Richter 1 sibling, 0 replies; 9+ messages in thread From: Johannes Weiner @ 2009-01-19 7:54 UTC (permalink / raw) To: Andrea Righi; +Cc: Dave Jones, Krzysztof Helt, Andrew Morton, LKML 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held 2009-01-18 20:17 ` [PATCH] fbmem: fix copy_from/to_user() with mutex held Andrea Righi 2009-01-19 7:54 ` Johannes Weiner @ 2009-01-19 7:58 ` Stefan Richter 2009-01-19 8:05 ` Stefan Richter 1 sibling, 1 reply; 9+ messages in thread From: Stefan Richter @ 2009-01-19 7:58 UTC (permalink / raw) To: righi.andrea Cc: Johannes Weiner, Dave Jones, Johannes Weiner, Krzysztof Helt, Andrew Morton, LKML 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held 2009-01-19 7:58 ` Stefan Richter @ 2009-01-19 8:05 ` Stefan Richter 2009-01-19 8:10 ` Harvey Harrison 0 siblings, 1 reply; 9+ messages in thread From: Stefan Richter @ 2009-01-19 8:05 UTC (permalink / raw) To: righi.andrea Cc: Johannes Weiner, Dave Jones, Johannes Weiner, Krzysztof Helt, Andrew Morton, LKML 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held 2009-01-19 8:05 ` Stefan Richter @ 2009-01-19 8:10 ` Harvey Harrison 2009-01-19 8:29 ` Andrea Righi 0 siblings, 1 reply; 9+ messages in thread From: Harvey Harrison @ 2009-01-19 8:10 UTC (permalink / raw) To: Stefan Richter Cc: righi.andrea, Johannes Weiner, Dave Jones, Johannes Weiner, Krzysztof Helt, Andrew Morton, LKML 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fbmem: fix copy_from/to_user() with mutex held 2009-01-19 8:10 ` Harvey Harrison @ 2009-01-19 8:29 ` Andrea Righi 0 siblings, 0 replies; 9+ messages in thread From: Andrea Righi @ 2009-01-19 8:29 UTC (permalink / raw) To: Harvey Harrison Cc: Stefan Richter, Johannes Weiner, Dave Jones, Johannes Weiner, Krzysztof Helt, Andrew Morton, LKML 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-01-19 8:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-01-17 23:19 .29rc2 lockdep report. fb_mmap vs sys_mmap2 Dave Jones 2009-01-18 2:00 ` Johannes Weiner 2009-01-18 19:21 ` Andrea Righi 2009-01-18 20:17 ` [PATCH] fbmem: fix copy_from/to_user() with mutex held Andrea Righi 2009-01-19 7:54 ` Johannes Weiner 2009-01-19 7:58 ` Stefan Richter 2009-01-19 8:05 ` Stefan Richter 2009-01-19 8:10 ` Harvey Harrison 2009-01-19 8:29 ` Andrea Righi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox