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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
