SRU Bug #104837 - kernel warns BUG: at fs/inotify.c:172 set_dentry_child_flags()

[Tim Gardner]

SRU Justification

Impact: There is a race between setting an inode's children's "parent watched" flag when placing the first watch on a parent, and instantiating new children of that parent: a child could miss having its flags set by set_dentry_child_flags, but then inotify_d_instantiate might still see !inotify_inode_watched.

Patch Description: The solution is to set_dentry_child_flags after adding the watch. Locking is taken care of, because both set_dentry_child_flags and inotify_d_instantiate hold dcache_lock and child->d_locks.

Patches: http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-hardy.git;a=commit;h=ab67a74144886e464f5b905558876145e711f17a, http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-hardy.git;a=commit;h=670dcd8597ed0aa7f68d05d384226bdb81b0e956

Test Case: See bug description.

>From ab67a74144886e464f5b905558876145e711f17a Mon Sep 17 00:00:00 2001
From: Nick Piggin <npiggin at suse.de>
Date: Wed, 6 Feb 2008 01:37:28 -0800
Subject: [PATCH] inotify: fix race
 Bug: #104837

There is a race between setting an inode's children's "parent watched" flag
when placing the first watch on a parent, and instantiating new children of
that parent: a child could miss having its flags set by
set_dentry_child_flags, but then inotify_d_instantiate might still see
!inotify_inode_watched.

The solution is to set_dentry_child_flags after adding the watch.  Locking is
taken care of, because both set_dentry_child_flags and inotify_d_instantiate
hold dcache_lock and child->d_locks.

Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
Signed-off-by: Nick Piggin <npiggin at suse.de>
Cc: Robert Love <rlove at google.com>
Cc: John McCutchan <ttb at tentacle.dhs.org>
Cc: Jan Kara <jack at ucw.cz>
Cc: Yan Zheng <yanzheng at 21cn.com>
Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
---
 fs/inotify.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/inotify.c b/fs/inotify.c
index 2c5b921..b2b109b 100644
--- a/fs/inotify.c
+++ b/fs/inotify.c
@@ -627,6 +627,7 @@ s32 inotify_add_watch(struct inotify_handle *ih, struct inotify_watch *watch,
 		      struct inode *inode, u32 mask)
 {
 	int ret = 0;
+	int newly_watched;
 
 	/* don't allow invalid bits: we don't want flags set */
 	mask &= IN_ALL_EVENTS | IN_ONESHOT;
@@ -653,12 +654,18 @@ s32 inotify_add_watch(struct inotify_handle *ih, struct inotify_watch *watch,
 	 */
 	watch->inode = igrab(inode);
 
-	if (!inotify_inode_watched(inode))
-		set_dentry_child_flags(inode, 1);
-
 	/* Add the watch to the handle's and the inode's list */
+	newly_watched = !inotify_inode_watched(inode);
 	list_add(&watch->h_list, &ih->watches);
 	list_add(&watch->i_list, &inode->inotify_watches);
+	/*
+	 * Set child flags _after_ adding the watch, so there is no race
+	 * windows where newly instantiated children could miss their parent's
+	 * watched flag.
+	 */
+	if (newly_watched)
+		set_dentry_child_flags(inode, 1);
+
 out:
 	mutex_unlock(&ih->mutex);
 	mutex_unlock(&inode->inotify_mutex);
-- 
1.5.4.3

>From 670dcd8597ed0aa7f68d05d384226bdb81b0e956 Mon Sep 17 00:00:00 2001
From: Nick Piggin <npiggin at suse.de>
Date: Wed, 6 Feb 2008 01:37:29 -0800
Subject: [PATCH] inotify: remove debug code
 Bug: #104837

The inotify debugging code is supposed to verify that the
DCACHE_INOTIFY_PARENT_WATCHED scalability optimisation does not result in
notifications getting lost nor extra needless locking generated.

Unfortunately there are also some races in the debugging code.  And it isn't
very good at finding problems anyway.  So remove it for now.

Signed-off-by: Nick Piggin <npiggin at suse.de>
Cc: Robert Love <rlove at google.com>
Cc: John McCutchan <ttb at tentacle.dhs.org>
Cc: Jan Kara <jack at ucw.cz>
Cc: Yan Zheng <yanzheng at 21cn.com>
Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
---
 fs/dcache.c  |    3 ---
 fs/inotify.c |   17 +++++------------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 6fc454e..7c718f9 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1408,9 +1408,6 @@ void d_delete(struct dentry * dentry)
 	if (atomic_read(&dentry->d_count) == 1) {
 		dentry_iput(dentry);
 		fsnotify_nameremove(dentry, isdir);
-
-		/* remove this and other inotify debug checks after 2.6.18 */
-		dentry->d_flags &= ~DCACHE_INOTIFY_PARENT_WATCHED;
 		return;
 	}
 
diff --git a/fs/inotify.c b/fs/inotify.c
index b2b109b..690e725 100644
--- a/fs/inotify.c
+++ b/fs/inotify.c
@@ -168,20 +168,14 @@ static void set_dentry_child_flags(struct inode *inode, int watched)
 		struct dentry *child;
 
 		list_for_each_entry(child, &alias->d_subdirs, d_u.d_child) {
-			if (!child->d_inode) {
-				WARN_ON(child->d_flags & DCACHE_INOTIFY_PARENT_WATCHED);
+			if (!child->d_inode)
 				continue;
-			}
+
 			spin_lock(&child->d_lock);
-			if (watched) {
-				WARN_ON(child->d_flags &
-						DCACHE_INOTIFY_PARENT_WATCHED);
+			if (watched)
 				child->d_flags |= DCACHE_INOTIFY_PARENT_WATCHED;
-			} else {
-				WARN_ON(!(child->d_flags &
-					DCACHE_INOTIFY_PARENT_WATCHED));
-				child->d_flags&=~DCACHE_INOTIFY_PARENT_WATCHED;
-			}
+			else
+				child->d_flags &=~DCACHE_INOTIFY_PARENT_WATCHED;
 			spin_unlock(&child->d_lock);
 		}
 	}
@@ -253,7 +247,6 @@ void inotify_d_instantiate(struct dentry *entry, struct inode *inode)
 	if (!inode)
 		return;
 
-	WARN_ON(entry->d_flags & DCACHE_INOTIFY_PARENT_WATCHED);
 	spin_lock(&entry->d_lock);
 	parent = entry->d_parent;
 	if (parent->d_inode && inotify_inode_watched(parent->d_inode))
-- 
1.5.4.3