Re: Fwd: Fw: [2.6.20.4] BUG: dentry xattrs still in use in shrink_dcache_for_umount()with reiserfs

[Andrea Righi]

Jeff Mahoney wrote:
> I have the patchset that I mentioned, but I'm not proposing it for 2.6.21.
> It's much too invasive to be introduced in an -rc7, but it does include
> locking changes that I believe avoid this bug.
> 
> Vladimir was right in his analysis that sometimes get_xa_root() takes the
> reference once and other times twice, but not for the right reasons.  I save
> a reference to the xattr dir to avoid a lookup later, but when there are multiple
> getxattrs or listxattrs as the first xattr operation on the file system, we can end
> up taking a second reference when we shouldn't. This is because those operations
> are protected by read locks and the ->xattr_root pointer isn't protected by anything
> else. A quick fix would be to just extend the protection of the priv root's i_mutex
> around the assignment, and test first. The right fix involves a complete rework of
> the locking, and I have code to do that, it's just too late to include it in 2.6.21.
> 
> I'd love to know what Andrea (and now Andi Kleen) are doing to hit this now. There
> haven't been any changes in this code in a while, and the shrink_dcache_for_umount()
> has been around since October. I'm unable to reproduce locally so far, so if Andrea
> or Andi could see if this fixes it for them, I'd appreciate it.

Jeff, your patch doesn't resolve, but you hit the problem. Actually, I
think the xattr_root pointer must be protected also in get_xa_root()
using the same private root i_mutex.

I've tested the following patch and it resolves in my case. What do you
think about it? could it be a reasonable fix?
 
Thanks!
-Andrea

--- linux/fs/reiserfs/xattr.c.orig	2007-04-14 18:53:02.000000000 +0200
+++ linux/fs/reiserfs/xattr.c	2007-04-21 14:16:02.000000000 +0200
@@ -72,14 +72,16 @@ static struct dentry *create_xa_root(str
 		err =
 		    privroot->d_inode->i_op->mkdir(privroot->d_inode, xaroot,
 						   0700);
-		mutex_unlock(&privroot->d_inode->i_mutex);
 
 		if (err) {
+			mutex_unlock(&privroot->d_inode->i_mutex);
 			dput(xaroot);
 			dput(privroot);
 			return ERR_PTR(err);
 		}
-		REISERFS_SB(sb)->xattr_root = dget(xaroot);
+		if (REISERFS_SB(sb)->xattr_root == NULL)
+			REISERFS_SB(sb)->xattr_root = dget(xaroot);
+		mutex_unlock(&privroot->d_inode->i_mutex);
 	}
 
       out:
@@ -122,12 +124,26 @@ static struct dentry *__get_xa_root(stru
  */
 static inline struct dentry *get_xa_root(struct super_block *s)
 {
-	struct dentry *dentry = dget(REISERFS_SB(s)->xattr_root);
+	struct dentry *privroot = dget(REISERFS_SB(s)->priv_root);
+	struct dentry *xaroot = NULL;
+
+	if (!privroot)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	if (!privroot->d_inode)
+		goto out;
+
+	mutex_lock(&privroot->d_inode->i_mutex);
+
+	xaroot = (REISERFS_SB(s)->xattr_root) ?
+			dget(REISERFS_SB(s)->xattr_root) :
+			__get_xa_root(s);
 
-	if (!dentry)
-		dentry = __get_xa_root(s);
+	mutex_unlock(&privroot->d_inode->i_mutex);
 
-	return dentry;
+      out:
+	dput(privroot);
+	return xaroot;
 }
 
 /* Opens the directory corresponding to the inode's extended attribute store.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/