lkml.org 
[lkml]   [2008]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subject[patch] mm: fix locking, inotify_read's ev_mutex vs do_page_fault's mmap_sem...

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > [ 6460.634452]
> > [ 6460.634465] =======================================================
> > [ 6460.634494] [ INFO: possible circular locking dependency detected ]
> > [ 6460.634517] 2.6.27-rc6-tip-00290-g8e229c3-dirty #1
> > [ 6460.634535] -------------------------------------------------------
> > [ 6460.634555] gdm-simple-gree/4778 is trying to acquire lock:
> > [ 6460.634574] (&mm->mmap_sem){----}, at: [<c018fe33>] might_fault+0x36/0x73
> > [ 6460.634639]
> > [ 6460.634645] but task is already holding lock:
> > [ 6460.634662] (&dev->ev_mutex){--..}, at: [<c01c7a76>] inotify_read+0xd8/0x16e
> > [ 6460.634715]
> > [ 6460.634721] which lock already depends on the new lock.
>
> Yes, there's a thread in my intray called "inotify_read's ev_mutex vs
> do_page_fault's mmap_sem...". It's a bit flakey-looking, but there's
> a patch in there.

ah, thx. I picked up the patch into tip/out-of-tree. (see below for a
tided up changelog) Please queue it up as v2.6.27 material. (i'll report
it if anything breaks due to the patch)

Ingo

--------------->
From 1eb0a42e4eb3283521ee1de99adbf567874b622f Mon Sep 17 00:00:00 2001
From: Nick Piggin <nickpiggin@yahoo.com.au>
Date: Thu, 11 Sep 2008 06:12:51 +1000
Subject: [PATCH] mm: fix locking, inotify_read's ev_mutex vs do_page_fault's mmap_sem...

Daniel J Blueman wrote:
> I observed this locking violation [1] while gnome-panel was loading;
> this was previously reported at
> http://uwsg.iu.edu/hypermail/linux/kernel/0806.3/2881.html .

Fix inotify lock order reversal with mmap_sem due to holding locks over
copy_to_user.

Reported-by: "Daniel J Blueman" <daniel.blueman@gmail.com>
Signed-off-by: Nick Piggin <npiggin@suse.de>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
fs/inotify_user.c | 27 ++++++++++++++++++++-------
include/asm-x86/uaccess_64.h | 1 +
2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/fs/inotify_user.c b/fs/inotify_user.c
index 6024942..d85c7d9 100644
--- a/fs/inotify_user.c
+++ b/fs/inotify_user.c
@@ -323,7 +323,7 @@ out:
}

/*
- * remove_kevent - cleans up and ultimately frees the given kevent
+ * remove_kevent - cleans up the given kevent
*
* Caller must hold dev->ev_mutex.
*/
@@ -334,7 +334,13 @@ static void remove_kevent(struct inotify_device *dev,

dev->event_count--;
dev->queue_size -= sizeof(struct inotify_event) + kevent->event.len;
+}

+/*
+ * free_kevent - frees the given kevent.
+ */
+static void free_kevent(struct inotify_kernel_event *kevent)
+{
kfree(kevent->name);
kmem_cache_free(event_cachep, kevent);
}
@@ -350,6 +356,7 @@ static void inotify_dev_event_dequeue(struct inotify_device *dev)
struct inotify_kernel_event *kevent;
kevent = inotify_dev_get_event(dev);
remove_kevent(dev, kevent);
+ free_kevent(kevent);
}
}

@@ -433,17 +440,15 @@ static ssize_t inotify_read(struct file *file, char __user *buf,
dev = file->private_data;

while (1) {
- int events;

prepare_to_wait(&dev->wq, &wait, TASK_INTERRUPTIBLE);

mutex_lock(&dev->ev_mutex);
- events = !list_empty(&dev->events);
- mutex_unlock(&dev->ev_mutex);
- if (events) {
+ if (!list_empty(&dev->events)) {
ret = 0;
break;
}
+ mutex_unlock(&dev->ev_mutex);

if (file->f_flags & O_NONBLOCK) {
ret = -EAGAIN;
@@ -462,7 +467,6 @@ static ssize_t inotify_read(struct file *file, char __user *buf,
if (ret)
return ret;

- mutex_lock(&dev->ev_mutex);
while (1) {
struct inotify_kernel_event *kevent;

@@ -481,6 +485,13 @@ static ssize_t inotify_read(struct file *file, char __user *buf,
}
break;
}
+ remove_kevent(dev, kevent);
+
+ /*
+ * Must perform the copy_to_user outside the mutex in order
+ * to avoid a lock order reversal with mmap_sem.
+ */
+ mutex_unlock(&dev->ev_mutex);

if (copy_to_user(buf, &kevent->event, event_size)) {
ret = -EFAULT;
@@ -498,7 +509,9 @@ static ssize_t inotify_read(struct file *file, char __user *buf,
count -= kevent->event.len;
}

- remove_kevent(dev, kevent);
+ free_kevent(kevent);
+
+ mutex_lock(&dev->ev_mutex);
}
mutex_unlock(&dev->ev_mutex);

diff --git a/include/asm-x86/uaccess_64.h b/include/asm-x86/uaccess_64.h
index 515d4dc..45806d6 100644
--- a/include/asm-x86/uaccess_64.h
+++ b/include/asm-x86/uaccess_64.h
@@ -7,6 +7,7 @@
#include <linux/compiler.h>
#include <linux/errno.h>
#include <linux/prefetch.h>
+#include <linux/lockdep.h>
#include <asm/page.h>

/*

\
 
 \ /
  Last update: 2008-09-14 10:09    [W:0.165 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site