Re: Problem with inotify

[Anton Altaparmakov <aia21@cam.ac.uk>]

Hi,

On Sat, 2 Jul 2005, Daniel Drake wrote:
> I'm trying to work around the NTFS lockup issue. Like others, I can reproduce
> it just by opening nautilus on an NTFS partition (i.e. creating an inotify
> watch on it) and then unmount at some point after - instant system lockup.
> 
> I have tried applying two patches:
> fix-soft-lockup-due-to-ntfs-vfs-part-and-explanation.patch from 2.6.13-rc1-mm1
> and the "NTFS: Fix a nasty deadlock that appeared in recent kernels" patch
> from your git tree.
> 
> However, I still get the freezing up on unmount. I am using 2.6.12, plus
> inotify-0.23-15, and the two patches mentioned above. Anything else I can try?

Thinking about it some more made me realize that there may be a problem in 
inotify after all...  Could you try the below patch to fs/inotify.c and 
tell me if it cures the lockup you are seeing?  (Note patch compiles but 
is otherwise untested.  But given it locks up without the patch it can't 
do much worse with it!)

Thanks a lot in advance!

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

inotify_unmount_inodes-list-iteration-fix.diff

Patch description: I believe that the inode reference that is being 
dropped by inotify's remove_watch() can cause inodes other than the 
current @inode to be moved away from the per-sb list.  And if this happens 
to be the next inode in the list, i.e. @next_i, then the iteration will 
proceed on the list that @next_i was moved to rather than the per-sb list.  
Thus, the check in the for loop (list_for_each_entry_safe()) for the @head 
being reached will _never_ be true and hence the for loop will keep going 
for ever...  Even worse the memory backing @next_i could be completely 
freed and then completely random results would be obtained.

Basically, I do not believe that using list_for_each_entry_safe() is safe 
at all as it only guards against removal of the current entry but not 
against removal of the next entry.  This patch tries to work around this 
by getting a reference to @next_i whilst the inode_lock is dropped.

Signed-off-by: Anton Altaparmakov <aia21@cantab.net>

--- linux-2.6.13-rc1-mm1-vanilla/fs/inotify.c	2005-07-01 14:51:09.000000000 +0100
+++ linux-2.6.13-rc1-mm1/fs/inotify.c	2005-07-02 22:11:11.000000000 +0100
@@ -560,9 +560,10 @@ EXPORT_SYMBOL_GPL(inotify_get_cookie);
  */
 void inotify_unmount_inodes(struct list_head *list)
 {
-	struct inode *inode, *next_i;
+	struct inode *inode, *next_i, *need_iput = NULL;
 
 	list_for_each_entry_safe(inode, next_i, list, i_sb_list) {
+		struct inode *need_iput_tmp;
 		struct inotify_watch *watch, *next_w;
 		struct list_head *watches;
 
@@ -574,8 +575,20 @@ void inotify_unmount_inodes(struct list_
 		if (inode->i_state & (I_CLEAR | I_FREEING))
 			continue;
 
+		need_iput_tmp = need_iput;
+		need_iput = NULL;
+
 		/* In case the remove_watch() drops a reference */
-		__iget(inode);
+		if (inode != need_iput_tmp)
+			__iget(inode);
+		else
+			need_iput_tmp = NULL;
+
+		/* In case the dropping of a reference would nuke next_i. */
+		if (!next_i->i_state & (I_CLEAR | I_FREEING)) {
+			__iget(next_i);
+			need_iput = next_i;
+		}
 
 		/*
 		 * We can safely drop inode_lock here because the per-sb list
@@ -584,6 +597,9 @@ void inotify_unmount_inodes(struct list_
 		 */
 		spin_unlock(&inode_lock);
 
+		if (need_iput_tmp)
+			iput(need_iput_tmp);
+
 		/* for each watch, send IN_UNMOUNT and then remove it */
 		down(&inode->inotify_sem);
 		watches = &inode->inotify_watches;
@@ -599,6 +615,11 @@ void inotify_unmount_inodes(struct list_
 
 		spin_lock(&inode_lock);
 	}
+	if (need_iput) {
+		spin_unlock(&inode_lock);
+		iput(need_iput);
+		spin_lock(&inode_lock);
+	}
 }
 EXPORT_SYMBOL_GPL(inotify_unmount_inodes);
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/