lockdep splat from fbmem

lockdep splat from fbmem

[Johannes Weiner]

Hi,

fb_mmap() is called under mmap_sem and acquires fb_info->lock.

Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
under it, which in turn might acquire mmap_sem.

Just noticed the trace and wanted to let you know.

        Hannes

Re: lockdep splat from fbmem

[Andrea Righi]

Johannes Weiner wrote:
> Hi,
> 
> fb_mmap() is called under mmap_sem and acquires fb_info->lock.
> 
> Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
> mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
> under it, which in turn might acquire mmap_sem.
> 
> Just noticed the trace and wanted to let you know.
> 
>         Hannes

Good catch. We could try to push down the mutex and call all the
copy_to/from_user() outside the lock. I can try look at it tonight,
if someone doesn't fix it before.

Thanks for reporting,
-Andrea

lockdep splat from ioctl and mmap fops sharing lock

[Johannes Weiner]

Johannes Weiner <hannes@saeurebad.de> writes:

> Hi,
>
> fb_mmap() is called under mmap_sem and acquires fb_info->lock.
>
> Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
> mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
> under it, which in turn might acquire mmap_sem.

More affected drivers:

agp_ioctl() doing usercopy under agp_fe.agp_mutex
agp_mmap() taking agp_fe.agp_mutex under mmap_sem

dv1394_write() doing usercopy under video->mtx
dv1394_mmap() taking video->mtx under mmap_sem

raw1394_ioctl() doing usercopy under fi->state_mutex
raw1394_mmap() taking fi->state_mutex under mmap_sem

	Hannes

Re: lockdep splat from ioctl and mmap fops sharing lock

[Stefan Richter]

Johannes Weiner wrote:
> Johannes Weiner <hannes@saeurebad.de> writes:
> 
>> Hi,
>>
>> fb_mmap() is called under mmap_sem and acquires fb_info->lock.
>>
>> Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
>> mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
>> under it, which in turn might acquire mmap_sem.
> 
> More affected drivers:
> 
> agp_ioctl() doing usercopy under agp_fe.agp_mutex
> agp_mmap() taking agp_fe.agp_mutex under mmap_sem
> 
> dv1394_write() doing usercopy under video->mtx
> dv1394_mmap() taking video->mtx under mmap_sem

OK, it doesn't surprise me that I didn't notice this one myself:  The
video->mtx usage is old, but dv1394 is not frequently used anymore; I
for one don't test anything of it beyond loading and unloading.

> raw1394_ioctl() doing usercopy under fi->state_mutex
> raw1394_mmap() taking fi->state_mutex under mmap_sem

The state_mutex in raw1394 however was introduced by me in patches
written against 2.6.26 and 2.6.27-rcs.  And I tested the ioctls and I'd
like to think that I also tested mmaps.  But maybe I didn't.  Of course
I ahve all sorts of lockdep options enabled.

So, was the usage of mmap_sem changed after 2.6.27 or were my tests
insufficient?
-- 
Stefan Richter
-=====-==--- =-=- =-===
http://arcgraph.de/sr/

Re: lockdep splat from ioctl and mmap fops sharing lock

[Johannes Weiner]

Stefan Richter <stefanr@s5r6.in-berlin.de> writes:

> Johannes Weiner wrote:
>> raw1394_ioctl() doing usercopy under fi->state_mutex
>> raw1394_mmap() taking fi->state_mutex under mmap_sem
>
> The state_mutex in raw1394 however was introduced by me in patches
> written against 2.6.26 and 2.6.27-rcs.  And I tested the ioctls and I'd
> like to think that I also tested mmaps.  But maybe I didn't.  Of course
> I ahve all sorts of lockdep options enabled.
>
> So, was the usage of mmap_sem changed after 2.6.27 or were my tests
> insufficient?

In linux-next/-mm, copy_to/from_user have lockdep annotations telling
that they might fault and therefor acquire the mmap_sem in #PF.

But since faults on ioctl parameters are so rare, without these
annotations you would probably never see a warning.

        Hannes

Re: lockdep splat from ioctl and mmap fops sharing lock

[Stefan Richter]

Johannes Weiner wrote:
> Stefan Richter <stefanr@s5r6.in-berlin.de> writes:
> 
>> Johannes Weiner wrote:
>>> raw1394_ioctl() doing usercopy under fi->state_mutex
>>> raw1394_mmap() taking fi->state_mutex under mmap_sem
...
>> was the usage of mmap_sem changed after 2.6.27 or were my tests
>> insufficient?
> 
> In linux-next/-mm, copy_to/from_user have lockdep annotations telling
> that they might fault and therefor acquire the mmap_sem in #PF.
> 
> But since faults on ioctl parameters are so rare, without these
> annotations you would probably never see a warning.

Thanks for the report and explanation.  I logged this as 
http://bugzilla.kernel.org/show_bug.cgi?id=11823 (dv1394, won't fix) and 
http://bugzilla.kernel.org/show_bug.cgi?id=11824 (raw1394, to be fixed 
in the 2.6.28-rc timeframe since it is a post 2.6.27 regression).
-- 
Stefan Richter
-=====-==--- =-=- ==--=
http://arcgraph.de/sr/

Re: lockdep splat from fbmem

[Andrea Righi]

Johannes Weiner wrote:
> Hi,
> 
> fb_mmap() is called under mmap_sem and acquires fb_info->lock.
> 
> Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
> mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
> under it, which in turn might acquire mmap_sem.
> 
> Just noticed the trace and wanted to let you know.
> 
>         Hannes

Johannes,

could you check the following fix for fbmem? maybe some parts are still
deadlockable, but I'm not able to raise any lockdep trace with it using
the latest Linus' git.

Thanks,
-Andrea

---
fb: avoid to call copy_from/to_user() with fb_info mutex held in fbmem ioctl()

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

diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
index 91b78e6..8f46589 100644
--- a/drivers/video/fbcmap.c
+++ b/drivers/video/fbcmap.c
@@ -245,14 +245,11 @@ int fb_set_cmap(struct fb_cmap *cmap, struct fb_info *info)
 	return rc;
 }
 
-int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
+int fb_set_user_cmap(struct fb_cmap_user *cmap, int fbidx)
 {
 	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;
+	struct fb_info *info;
 
 	memset(&umap, 0, sizeof(struct fb_cmap));
 	rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);
@@ -262,11 +259,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(fbidx);
+	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 cd5f20d..b4de3f3 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1002,6 +1002,24 @@ fb_blank(struct fb_info *info, int blank)
  	return ret;
 }
 
+struct fb_info *get_fb_info(int fbidx)
+{
+	struct fb_info *info;
+
+	info = registered_fb[fbidx];
+	mutex_lock(&info->lock);
+	if (!info->fbops) {
+		mutex_unlock(&info->lock);
+		return NULL;
+	}
+	return info;
+}
+
+void put_fb_info(struct fb_info *info)
+{
+	mutex_unlock(&info->lock);
+}
+
 static long
 fb_ioctl(struct file *file, unsigned int cmd,
 	 unsigned long arg)
@@ -1013,120 +1031,138 @@ fb_ioctl(struct file *file, 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;
 
-	info = registered_fb[fbidx];
-	mutex_lock(&info->lock);
-	fb = info->fbops;
-
-	if (!fb) {
-		mutex_unlock(&info->lock);
-		return -ENODEV;
-	}
 	switch (cmd) {
 	case FBIOGET_VSCREENINFO:
-		ret = copy_to_user(argp, &info->var,
-				    sizeof(var)) ? -EFAULT : 0;
+		info = get_fb_info(fbidx);
+		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(fbidx);
+		if (!info)
+			return -ENODEV;
 		acquire_console_sem();
 		info->flags |= FBINFO_MISC_USEREVENT;
 		ret = fb_set_var(info, &var);
 		info->flags &= ~FBINFO_MISC_USEREVENT;
 		release_console_sem();
+		put_fb_info(info);
 		if (ret == 0 && copy_to_user(argp, &var, sizeof(var)))
-			ret = -EFAULT;
+			return -EFAULT;
 		break;
 	case FBIOGET_FSCREENINFO:
-		ret = copy_to_user(argp, &info->fix,
-				    sizeof(fix)) ? -EFAULT : 0;
+		info = get_fb_info(fbidx);
+		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, fbidx);
 		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(fbidx);
+		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(fbidx);
+		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(fbidx);
+		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(fbidx);
+		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(fbidx);
+		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();
+		put_fb_info(info);
 		break;;
 	default:
-		if (fb->fb_ioctl == NULL)
-			ret = -ENOTTY;
-		else
+		info = get_fb_info(fbidx);
+		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);
 	}
-	mutex_unlock(&info->lock);
 	return ret;
 }
 
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 75a81ea..9f4e70f 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(int fbidx);
+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, int fbidx);
 extern const struct fb_cmap *fb_default_cmap(int len);
 extern void fb_invert_cmaps(void);