[2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Daniel J Blueman]

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 .

Let me know for more information/config/testing. Thanks!
  Daniel

--- [1]

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.27-rc5-233c-debug #2
-------------------------------------------------------
gnome-panel/4944 is trying to acquire lock:
 (&mm->mmap_sem){----}, at: [<ffffffff806aa40f>] do_page_fault+0x12f/0xae0

but task is already holding lock:
 (&dev->ev_mutex){--..}, at: [<ffffffff80320ac3>] inotify_read+0xe3/0x200

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #3 (&dev->ev_mutex){--..}:
       [<ffffffff80271889>] __lock_acquire+0xd49/0x1190
       [<ffffffff80271d61>] lock_acquire+0x91/0xc0
       [<ffffffff806a5069>] __mutex_lock_common+0xb9/0x430
       [<ffffffff806a54bf>] mutex_lock_nested+0x3f/0x50
       [<ffffffff80321216>] inotify_dev_queue_event+0x46/0x1c0
       [<ffffffff803200e6>] inotify_inode_queue_event+0xc6/0x110
       [<ffffffff802f424c>] fsnotify_create+0x3c/0x70
       [<ffffffff802f4c5d>] vfs_create+0xbd/0xd0
       [<ffffffff802f7dfe>] do_filp_open+0x80e/0x910
       [<ffffffff802e82b0>] do_sys_open+0x80/0x110
       [<ffffffff802e8380>] sys_open+0x20/0x30
       [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #2 (&ih->mutex){--..}:
       [<ffffffff80271889>] __lock_acquire+0xd49/0x1190
       [<ffffffff80271d61>] lock_acquire+0x91/0xc0
       [<ffffffff806a5069>] __mutex_lock_common+0xb9/0x430
       [<ffffffff806a54bf>] mutex_lock_nested+0x3f/0x50
       [<ffffffff8031fe73>] inotify_find_update_watch+0x53/0xe0
       [<ffffffff80320d85>] sys_inotify_add_watch+0x115/0x1d0
       [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #1 (&inode->inotify_mutex){--..}:
       [<ffffffff80271889>] __lock_acquire+0xd49/0x1190
       [<ffffffff80271d61>] lock_acquire+0x91/0xc0
       [<ffffffff806a5069>] __mutex_lock_common+0xb9/0x430
       [<ffffffff806a54bf>] mutex_lock_nested+0x3f/0x50
       [<ffffffff80320070>] inotify_inode_queue_event+0x50/0x110
       [<ffffffff803206e1>] inotify_dentry_parent_queue_event+0x91/0xb0
       [<ffffffff802ebb7f>] __fput+0x7f/0x1f0
       [<ffffffff802ebd15>] fput+0x25/0x30
       [<ffffffff802c84ff>] remove_vma+0x4f/0x90
       [<ffffffff802ca359>] do_munmap+0x2e9/0x330
       [<ffffffff802ca3f5>] sys_munmap+0x55/0x80
       [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #0 (&mm->mmap_sem){----}:
       [<ffffffff80271950>] __lock_acquire+0xe10/0x1190
       [<ffffffff80271d61>] lock_acquire+0x91/0xc0
       [<ffffffff806a56eb>] down_read+0x4b/0x80
       [<ffffffff806aa40f>] do_page_fault+0x12f/0xae0
       [<ffffffff806a7bdd>] error_exit+0x0/0xa9
       [<ffffffff802eac08>] vfs_read+0xc8/0x170
       [<ffffffff802eadb5>] sys_read+0x55/0x90
       [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

other info that might help us debug this:

1 lock held by gnome-panel/4944:
 #0:  (&dev->ev_mutex){--..}, at: [<ffffffff80320ac3>] inotify_read+0xe3/0x200

stack backtrace:
Pid: 4944, comm: gnome-panel Not tainted 2.6.27-rc5-233c-debug #2

Call Trace:
 [<ffffffff8026f807>] print_circular_bug_tail+0xa7/0xf0
 [<ffffffff80271950>] __lock_acquire+0xe10/0x1190
 [<ffffffff80271d61>] lock_acquire+0x91/0xc0
 [<ffffffff806aa40f>] ? do_page_fault+0x12f/0xae0
 [<ffffffff806a56eb>] down_read+0x4b/0x80
 [<ffffffff806aa40f>] ? do_page_fault+0x12f/0xae0
 [<ffffffff8025c51a>] ? search_exception_tables+0x2a/0x50
 [<ffffffff806aa40f>] do_page_fault+0x12f/0xae0
 [<ffffffff8026d951>] ? trace_hardirqs_off_caller+0x21/0xc0
 [<ffffffff80213fc0>] ? native_sched_clock+0x90/0xb0
 [<ffffffff80270e39>] ? __lock_acquire+0x2f9/0x1190
 [<ffffffff80270606>] ? mark_held_locks+0x56/0xa0
 [<ffffffff802708bd>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff80270849>] ? trace_hardirqs_on_caller+0x149/0x1b0
 [<ffffffff806a7bdd>] error_exit+0x0/0xa9
 [<ffffffff80459acd>] ? copy_user_generic_string+0x2d/0x40
 [<ffffffff80320b5d>] ? inotify_read+0x17d/0x200
 [<ffffffff8025ee00>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff802708bd>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff802eac08>] vfs_read+0xc8/0x170
 [<ffffffff802eadb5>] sys_read+0x55/0x90
 [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
-- 
Daniel J Blueman

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 5482 bytes --]

On Wednesday 10 September 2008 07:03, 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 .
>
> Let me know for more information/config/testing. Thanks!

Thanks for the report. I've attached a patch you could test. It compiles
(and boots a UML here) but I don't think I've actually tested the inotify
path at all, so it may explode on you.

Peter, this copy_*_user stuff is quite a nightmare... Well actually it
isn't, if the code is designed with it in mind from the start, but it is
easy for people to forget it can take mmap_sem and filesystem locks... Is
there a way to annotate it and say "might take mmap_sem for read" for
example? So that these LORs will _always_ trigger rather than just once
in a million times when the reclaim gods frown on us?

Anyway, Daniel, thanks again...


>   Daniel
>
> --- [1]
>
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.27-rc5-233c-debug #2
> -------------------------------------------------------
> gnome-panel/4944 is trying to acquire lock:
>  (&mm->mmap_sem){----}, at: [<ffffffff806aa40f>] do_page_fault+0x12f/0xae0
>
> but task is already holding lock:
>  (&dev->ev_mutex){--..}, at: [<ffffffff80320ac3>] inotify_read+0xe3/0x200
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #3 (&dev->ev_mutex){--..}:
>        [<ffffffff80271889>] __lock_acquire+0xd49/0x1190
>        [<ffffffff80271d61>] lock_acquire+0x91/0xc0
>        [<ffffffff806a5069>] __mutex_lock_common+0xb9/0x430
>        [<ffffffff806a54bf>] mutex_lock_nested+0x3f/0x50
>        [<ffffffff80321216>] inotify_dev_queue_event+0x46/0x1c0
>        [<ffffffff803200e6>] inotify_inode_queue_event+0xc6/0x110
>        [<ffffffff802f424c>] fsnotify_create+0x3c/0x70
>        [<ffffffff802f4c5d>] vfs_create+0xbd/0xd0
>        [<ffffffff802f7dfe>] do_filp_open+0x80e/0x910
>        [<ffffffff802e82b0>] do_sys_open+0x80/0x110
>        [<ffffffff802e8380>] sys_open+0x20/0x30
>        [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
>        [<ffffffffffffffff>] 0xffffffffffffffff
>
> -> #2 (&ih->mutex){--..}:
>        [<ffffffff80271889>] __lock_acquire+0xd49/0x1190
>        [<ffffffff80271d61>] lock_acquire+0x91/0xc0
>        [<ffffffff806a5069>] __mutex_lock_common+0xb9/0x430
>        [<ffffffff806a54bf>] mutex_lock_nested+0x3f/0x50
>        [<ffffffff8031fe73>] inotify_find_update_watch+0x53/0xe0
>        [<ffffffff80320d85>] sys_inotify_add_watch+0x115/0x1d0
>        [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
>        [<ffffffffffffffff>] 0xffffffffffffffff
>
> -> #1 (&inode->inotify_mutex){--..}:
>        [<ffffffff80271889>] __lock_acquire+0xd49/0x1190
>        [<ffffffff80271d61>] lock_acquire+0x91/0xc0
>        [<ffffffff806a5069>] __mutex_lock_common+0xb9/0x430
>        [<ffffffff806a54bf>] mutex_lock_nested+0x3f/0x50
>        [<ffffffff80320070>] inotify_inode_queue_event+0x50/0x110
>        [<ffffffff803206e1>] inotify_dentry_parent_queue_event+0x91/0xb0
>        [<ffffffff802ebb7f>] __fput+0x7f/0x1f0
>        [<ffffffff802ebd15>] fput+0x25/0x30
>        [<ffffffff802c84ff>] remove_vma+0x4f/0x90
>        [<ffffffff802ca359>] do_munmap+0x2e9/0x330
>        [<ffffffff802ca3f5>] sys_munmap+0x55/0x80
>        [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
>        [<ffffffffffffffff>] 0xffffffffffffffff
>
> -> #0 (&mm->mmap_sem){----}:
>        [<ffffffff80271950>] __lock_acquire+0xe10/0x1190
>        [<ffffffff80271d61>] lock_acquire+0x91/0xc0
>        [<ffffffff806a56eb>] down_read+0x4b/0x80
>        [<ffffffff806aa40f>] do_page_fault+0x12f/0xae0
>        [<ffffffff806a7bdd>] error_exit+0x0/0xa9
>        [<ffffffff802eac08>] vfs_read+0xc8/0x170
>        [<ffffffff802eadb5>] sys_read+0x55/0x90
>        [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b
>        [<ffffffffffffffff>] 0xffffffffffffffff
>
> other info that might help us debug this:
>
> 1 lock held by gnome-panel/4944:
>  #0:  (&dev->ev_mutex){--..}, at: [<ffffffff80320ac3>]
> inotify_read+0xe3/0x200
>
> stack backtrace:
> Pid: 4944, comm: gnome-panel Not tainted 2.6.27-rc5-233c-debug #2
>
> Call Trace:
>  [<ffffffff8026f807>] print_circular_bug_tail+0xa7/0xf0
>  [<ffffffff80271950>] __lock_acquire+0xe10/0x1190
>  [<ffffffff80271d61>] lock_acquire+0x91/0xc0
>  [<ffffffff806aa40f>] ? do_page_fault+0x12f/0xae0
>  [<ffffffff806a56eb>] down_read+0x4b/0x80
>  [<ffffffff806aa40f>] ? do_page_fault+0x12f/0xae0
>  [<ffffffff8025c51a>] ? search_exception_tables+0x2a/0x50
>  [<ffffffff806aa40f>] do_page_fault+0x12f/0xae0
>  [<ffffffff8026d951>] ? trace_hardirqs_off_caller+0x21/0xc0
>  [<ffffffff80213fc0>] ? native_sched_clock+0x90/0xb0
>  [<ffffffff80270e39>] ? __lock_acquire+0x2f9/0x1190
>  [<ffffffff80270606>] ? mark_held_locks+0x56/0xa0
>  [<ffffffff802708bd>] ? trace_hardirqs_on+0xd/0x10
>  [<ffffffff80270849>] ? trace_hardirqs_on_caller+0x149/0x1b0
>  [<ffffffff806a7bdd>] error_exit+0x0/0xa9
>  [<ffffffff80459acd>] ? copy_user_generic_string+0x2d/0x40
>  [<ffffffff80320b5d>] ? inotify_read+0x17d/0x200
>  [<ffffffff8025ee00>] ? autoremove_wake_function+0x0/0x40
>  [<ffffffff802708bd>] ? trace_hardirqs_on+0xd/0x10
>  [<ffffffff802eac08>] vfs_read+0xc8/0x170
>  [<ffffffff802eadb5>] sys_read+0x55/0x90
>  [<ffffffff8020c86b>] system_call_fastpath+0x16/0x1b

[-- Attachment #2: inotify-lor-fix.patch --]
[-- Type: text/x-diff, Size: 2356 bytes --]

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

Signed-off-by: Nick Piggin <npiggin@suse.de>
---
Index: linux-2.6/fs/inotify_user.c
===================================================================
--- linux-2.6.orig/fs/inotify_user.c
+++ linux-2.6/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
 
 	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(st
 		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 
 	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;
+			goto have_events;
 		}
+		mutex_unlock(&dev->ev_mutex);
 
 		if (file->f_flags & O_NONBLOCK) {
 			ret = -EAGAIN;
@@ -459,10 +464,9 @@ static ssize_t inotify_read(struct file 
 	}
 
 	finish_wait(&dev->wq, &wait);
-	if (ret)
-		return ret;
+	return ret;
 
-	mutex_lock(&dev->ev_mutex);
+have_events:
 	while (1) {
 		struct inotify_kernel_event *kevent;
 
@@ -481,6 +485,13 @@ static ssize_t inotify_read(struct file 
 			}
 			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 
 			count -= kevent->event.len;
 		}
 
-		remove_kevent(dev, kevent);
+		free_kevent(kevent);
+
+		mutex_lock(&dev->ev_mutex);
 	}
 	mutex_unlock(&dev->ev_mutex);
 

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Peter Zijlstra]

On Wed, 2008-09-10 at 14:07 +1000, Nick Piggin wrote:
> On Wednesday 10 September 2008 07:03, 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 .
> >
> > Let me know for more information/config/testing. Thanks!
> 
> Thanks for the report. I've attached a patch you could test. It compiles
> (and boots a UML here) but I don't think I've actually tested the inotify
> path at all, so it may explode on you.
> 
> Peter, this copy_*_user stuff is quite a nightmare... Well actually it
> isn't, if the code is designed with it in mind from the start, but it is
> easy for people to forget it can take mmap_sem and filesystem locks... Is
> there a way to annotate it and say "might take mmap_sem for read" for
> example? So that these LORs will _always_ trigger rather than just once
> in a million times when the reclaim gods frown on us?

Sure, how about the below - untested - uncompiled, might eat kittens,
etc..

Just sprinkle something like:

  might_lock_read(&mm->mmap_sem);

in the right places.

---
Subject: lockdep: might_lock annotation

useful to establish a lock dependency in case the actual dependency is
rare or hard to trigger.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 331e5f1..0aa657a 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -480,4 +480,22 @@ static inline void print_irqtrace_events(struct task_struct *curr)
 # define lock_map_release(l)			do { } while (0)
 #endif
 
+#ifdef CONFIG_PROVE_LOCKING
+# define might_lock(lock) 						\
+do {									\
+	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
+	lock_acquire(&(lock)->dep_map, 0, 0, 0, 2, NULL, _THIS_IP_);	\
+	lock_release(&(lock)->dep_map, 0, _THIS_IP_);			\
+} while (0)
+# define might_lock_read(lock) 						\
+do {									\
+	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
+	lock_acquire(&(lock)->dep_map, 0, 0, 1, 2, NULL, _THIS_IP_);	\
+	lock_release(&(lock)->dep_map, 0, _THIS_IP_);			\
+} while (0)
+#else
+# define might_lock(lock) do { } while (0)
+# define might_lock_read(lock) do { } while (0)
+#endif
+
 #endif /* __LINUX_LOCKDEP_H */

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Nick Piggin]

On Wednesday 10 September 2008 17:57, Peter Zijlstra wrote:
> On Wed, 2008-09-10 at 14:07 +1000, Nick Piggin wrote:
> > On Wednesday 10 September 2008 07:03, 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 .
> > >
> > > Let me know for more information/config/testing. Thanks!
> >
> > Thanks for the report. I've attached a patch you could test. It compiles
> > (and boots a UML here) but I don't think I've actually tested the inotify
> > path at all, so it may explode on you.
> >
> > Peter, this copy_*_user stuff is quite a nightmare... Well actually it
> > isn't, if the code is designed with it in mind from the start, but it is
> > easy for people to forget it can take mmap_sem and filesystem locks... Is
> > there a way to annotate it and say "might take mmap_sem for read" for
> > example? So that these LORs will _always_ trigger rather than just once
> > in a million times when the reclaim gods frown on us?
>
> Sure, how about the below - untested - uncompiled, might eat kittens,
> etc..
>
> Just sprinkle something like:
>
>   might_lock_read(&mm->mmap_sem);
>
> in the right places.

Ahh, very nice, thanks! I'll give that a try...


> ---
> Subject: lockdep: might_lock annotation
>
> useful to establish a lock dependency in case the actual dependency is
> rare or hard to trigger.
>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 331e5f1..0aa657a 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -480,4 +480,22 @@ static inline void print_irqtrace_events(struct
> task_struct *curr) # define lock_map_release(l)			do { } while (0)
>  #endif
>
> +#ifdef CONFIG_PROVE_LOCKING
> +# define might_lock(lock) 						\
> +do {									\
> +	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
> +	lock_acquire(&(lock)->dep_map, 0, 0, 0, 2, NULL, _THIS_IP_);	\
> +	lock_release(&(lock)->dep_map, 0, _THIS_IP_);			\
> +} while (0)
> +# define might_lock_read(lock) 						\
> +do {									\
> +	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
> +	lock_acquire(&(lock)->dep_map, 0, 0, 1, 2, NULL, _THIS_IP_);	\
> +	lock_release(&(lock)->dep_map, 0, _THIS_IP_);			\
> +} while (0)
> +#else
> +# define might_lock(lock) do { } while (0)
> +# define might_lock_read(lock) do { } while (0)
> +#endif
> +
>  #endif /* __LINUX_LOCKDEP_H */

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Ingo Molnar]

* Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> On Wednesday 10 September 2008 17:57, Peter Zijlstra wrote:
> > On Wed, 2008-09-10 at 14:07 +1000, Nick Piggin wrote:
> > > On Wednesday 10 September 2008 07:03, 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 .
> > > >
> > > > Let me know for more information/config/testing. Thanks!
> > >
> > > Thanks for the report. I've attached a patch you could test. It compiles
> > > (and boots a UML here) but I don't think I've actually tested the inotify
> > > path at all, so it may explode on you.
> > >
> > > Peter, this copy_*_user stuff is quite a nightmare... Well actually it
> > > isn't, if the code is designed with it in mind from the start, but it is
> > > easy for people to forget it can take mmap_sem and filesystem locks... Is
> > > there a way to annotate it and say "might take mmap_sem for read" for
> > > example? So that these LORs will _always_ trigger rather than just once
> > > in a million times when the reclaim gods frown on us?
> >
> > Sure, how about the below - untested - uncompiled, might eat kittens,
> > etc..
> >
> > Just sprinkle something like:
> >
> >   might_lock_read(&mm->mmap_sem);
> >
> > in the right places.
> 
> Ahh, very nice, thanks! I'll give that a try...

cool! Please send in an RFC patch once you have something that boots - 
we can stick it into tip/core/locking and see whether there's any new 
messages on a wide range of systems and workloads. (and we'd also check 
whether the number of kittens is an invariant.)

	Ingo

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Nick Piggin]

On Wednesday 10 September 2008 18:37, Ingo Molnar wrote:
> * Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > On Wednesday 10 September 2008 17:57, Peter Zijlstra wrote:

> > > Sure, how about the below - untested - uncompiled, might eat kittens,
> > > etc..
> > >
> > > Just sprinkle something like:
> > >
> > >   might_lock_read(&mm->mmap_sem);
> > >
> > > in the right places.
> >
> > Ahh, very nice, thanks! I'll give that a try...
>
> cool! Please send in an RFC patch once you have something that boots -
> we can stick it into tip/core/locking and see whether there's any new
> messages on a wide range of systems and workloads. (and we'd also check
> whether the number of kittens is an invariant.)

Well I have verified it boots, and have used the annotation in some of
x86-64's user copy routines (luckily no flood of bugs I was scared of,
phew!)

So I would like to request you merge Peter's patch, and we'll hopefully
start seeing the annotations being used. FWIW, I don't suppose lockdep
can determine that it is a sleeping lock, and do the appropriate
might_sleep checks at this point as well?

Thanks,
Nick

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Peter Zijlstra]

On Wed, 2008-09-10 at 19:50 +1000, Nick Piggin wrote:
> On Wednesday 10 September 2008 18:37, Ingo Molnar wrote:
> > * Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > > On Wednesday 10 September 2008 17:57, Peter Zijlstra wrote:
> 
> > > > Sure, how about the below - untested - uncompiled, might eat kittens,
> > > > etc..
> > > >
> > > > Just sprinkle something like:
> > > >
> > > >   might_lock_read(&mm->mmap_sem);
> > > >
> > > > in the right places.
> > >
> > > Ahh, very nice, thanks! I'll give that a try...
> >
> > cool! Please send in an RFC patch once you have something that boots -
> > we can stick it into tip/core/locking and see whether there's any new
> > messages on a wide range of systems and workloads. (and we'd also check
> > whether the number of kittens is an invariant.)
> 
> Well I have verified it boots, and have used the annotation in some of
> x86-64's user copy routines (luckily no flood of bugs I was scared of,
> phew!)
> 
> So I would like to request you merge Peter's patch, and we'll hopefully
> start seeing the annotations being used. 

Will you send your x86_64 patch to be the first?

> FWIW, I don't suppose lockdep
> can determine that it is a sleeping lock, and do the appropriate
> might_sleep checks at this point as well?

Humm, no - we don't actually have that information there - I guess one
could add it to lockdep_map and set it from the various init routines,
but I'm not sure its worth it - just add might_sleep() along with
might_lock() :-)

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Nick Piggin]

On Wednesday 10 September 2008 19:59, Peter Zijlstra wrote:
> On Wed, 2008-09-10 at 19:50 +1000, Nick Piggin wrote:
> > On Wednesday 10 September 2008 18:37, Ingo Molnar wrote:
> > > * Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > > > On Wednesday 10 September 2008 17:57, Peter Zijlstra wrote:
> > > > > Sure, how about the below - untested - uncompiled, might eat
> > > > > kittens, etc..
> > > > >
> > > > > Just sprinkle something like:
> > > > >
> > > > >   might_lock_read(&mm->mmap_sem);
> > > > >
> > > > > in the right places.
> > > >
> > > > Ahh, very nice, thanks! I'll give that a try...
> > >
> > > cool! Please send in an RFC patch once you have something that boots -
> > > we can stick it into tip/core/locking and see whether there's any new
> > > messages on a wide range of systems and workloads. (and we'd also check
> > > whether the number of kittens is an invariant.)
> >
> > Well I have verified it boots, and have used the annotation in some of
> > x86-64's user copy routines (luckily no flood of bugs I was scared of,
> > phew!)
> >
> > So I would like to request you merge Peter's patch, and we'll hopefully
> > start seeing the annotations being used.
>
> Will you send your x86_64 patch to be the first?

OK.


> > FWIW, I don't suppose lockdep
> > can determine that it is a sleeping lock, and do the appropriate
> > might_sleep checks at this point as well?
>
> Humm, no - we don't actually have that information there - I guess one
> could add it to lockdep_map and set it from the various init routines,
> but I'm not sure its worth it - just add might_sleep() along with
> might_lock() :-)

I guess so... seems like lockdep should know about "locks", however :)
Spinlock isn't always a spinlock, and ordinary kernel code usually
should not care about that.

But I'll add it in this case.

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On Wednesday 10 September 2008 14:07, Nick Piggin wrote:
> On Wednesday 10 September 2008 07:03, 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 .
> >
> > Let me know for more information/config/testing. Thanks!
>
> Thanks for the report. I've attached a patch you could test. It compiles
> (and boots a UML here) but I don't think I've actually tested the inotify
> path at all, so it may explode on you.

Indeed it was wrong, I messed up the waitqueue handling. This one
survives longer for me...

[-- Attachment #2: inotify-lor-fix.patch --]
[-- Type: text/x-diff, Size: 2629 bytes --]

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

Signed-off-by: Nick Piggin <npiggin@suse.de>
---
Index: linux-2.6/fs/inotify_user.c
===================================================================
--- linux-2.6.orig/fs/inotify_user.c
+++ linux-2.6/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
 
 	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(st
 		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 
 	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 
 	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 
 			}
 			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 
 			count -= kevent->event.len;
 		}
 
-		remove_kevent(dev, kevent);
+		free_kevent(kevent);
+
+		mutex_lock(&dev->ev_mutex);
 	}
 	mutex_unlock(&dev->ev_mutex);
 
Index: linux-2.6/include/asm-x86/uaccess_64.h
===================================================================
--- linux-2.6.orig/include/asm-x86/uaccess_64.h
+++ linux-2.6/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>
 
 /*

Re: [2.6.27-rc5] inotify_read's ev_mutex vs do_page_fault's mmap_sem...

[Daniel J Blueman]

On Wed, Sep 10, 2008 at 9:12 PM, Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> On Wednesday 10 September 2008 14:07, Nick Piggin wrote:
>> On Wednesday 10 September 2008 07:03, 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 .
>> >
>> > Let me know for more information/config/testing. Thanks!
>>
>> Thanks for the report. I've attached a patch you could test. It compiles
>> (and boots a UML here) but I don't think I've actually tested the inotify
>> path at all, so it may explode on you.
>
> Indeed it was wrong, I messed up the waitqueue handling. This one
> survives longer for me...

So far, so good with that last patch. I'll continue testing this and
follow up if I find any problem.

Thanks for the patching, Nick!

Daniel
-- 
Daniel J Blueman