[PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Jeff Mahoney]

 2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
 bugs by ensuring that lookup_one_len is always called under i_mutex.

 This patch expands the i_mutex locking to enclose lookup_one_len. This was
 always required, but not not enforced in the reiserfs code since it
 does locking around the xattr interactions with the xattr_sem.

 This is obvious enough, but it survived an overnight 50 thread ACL test.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/reiserfs/xattr.c |   32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -120,25 +120,20 @@ static struct dentry *lookup_or_create_d
 	struct dentry *dentry;
 	BUG_ON(!parent);
 
+	mutex_lock_nested(&parent->d_inode->i_mutex, I_MUTEX_XATTR);
 	dentry = lookup_one_len(name, parent, strlen(name));
-	if (IS_ERR(dentry))
-		return dentry;
-	else if (!dentry->d_inode) {
+	if (!IS_ERR(dentry) && !dentry->d_inode) {
 		int err = -ENODATA;
 
-		if (xattr_may_create(flags)) {
-			mutex_lock_nested(&parent->d_inode->i_mutex,
-					  I_MUTEX_XATTR);
+		if (xattr_may_create(flags))
 			err = xattr_mkdir(parent->d_inode, dentry, 0700);
-			mutex_unlock(&parent->d_inode->i_mutex);
-		}
 
 		if (err) {
 			dput(dentry);
 			dentry = ERR_PTR(err);
 		}
 	}
-
+	mutex_unlock(&parent->d_inode->i_mutex);
 	return dentry;
 }
 
@@ -184,6 +179,7 @@ fill_with_dentries(void *buf, const char
 {
 	struct reiserfs_dentry_buf *dbuf = buf;
 	struct dentry *dentry;
+	WARN_ON_ONCE(!mutex_is_locked(&dbuf->xadir->d_inode->i_mutex));
 
 	if (dbuf->count == ARRAY_SIZE(dbuf->dentries))
 		return -ENOSPC;
@@ -349,6 +345,7 @@ static struct dentry *xattr_lookup(struc
 	if (IS_ERR(xadir))
 		return ERR_CAST(xadir);
 
+	mutex_lock_nested(&xadir->d_inode->i_mutex, I_MUTEX_XATTR);
 	xafile = lookup_one_len(name, xadir, strlen(name));
 	if (IS_ERR(xafile)) {
 		err = PTR_ERR(xafile);
@@ -360,18 +357,15 @@ static struct dentry *xattr_lookup(struc
 
 	if (!xafile->d_inode) {
 		err = -ENODATA;
-		if (xattr_may_create(flags)) {
-			mutex_lock_nested(&xadir->d_inode->i_mutex,
-					  I_MUTEX_XATTR);
+		if (xattr_may_create(flags))
 			err = xattr_create(xadir->d_inode, xafile,
 					      0700|S_IFREG);
-			mutex_unlock(&xadir->d_inode->i_mutex);
-		}
 	}
 
 	if (err)
 		dput(xafile);
 out:
+	mutex_unlock(&xadir->d_inode->i_mutex);
 	dput(xadir);
 	if (err)
 		return ERR_PTR(err);
@@ -435,6 +429,7 @@ static int lookup_and_delete_xattr(struc
 	if (IS_ERR(xadir))
 		return PTR_ERR(xadir);
 
+	mutex_lock_nested(&xadir->d_inode->i_mutex, I_MUTEX_XATTR);
 	dentry = lookup_one_len(name, xadir, strlen(name));
 	if (IS_ERR(dentry)) {
 		err = PTR_ERR(dentry);
@@ -442,14 +437,13 @@ static int lookup_and_delete_xattr(struc
 	}
 
 	if (dentry->d_inode) {
-		mutex_lock_nested(&xadir->d_inode->i_mutex, I_MUTEX_XATTR);
 		err = xattr_unlink(xadir->d_inode, dentry);
-		mutex_unlock(&xadir->d_inode->i_mutex);
 		update_ctime(inode);
 	}
 
 	dput(dentry);
 out_dput:
+	mutex_unlock(&xadir->d_inode->i_mutex);
 	dput(xadir);
 	return err;
 }
@@ -906,9 +900,9 @@ static int create_privroot(struct dentry
 {
 	int err;
 	struct inode *inode = dentry->d_parent->d_inode;
-	mutex_lock_nested(&inode->i_mutex, I_MUTEX_XATTR);
+	WARN_ON_ONCE(!mutex_is_locked(&inode->i_mutex));
+
 	err = xattr_mkdir(inode, dentry, 0700);
-	mutex_unlock(&inode->i_mutex);
 	if (err) {
 		dput(dentry);
 		dentry = NULL;
@@ -980,6 +974,7 @@ int reiserfs_xattr_init(struct super_blo
 	/* If we don't have the privroot located yet - go find it */
 	if (!REISERFS_SB(s)->priv_root) {
 		struct dentry *dentry;
+		mutex_lock_nested(&s->s_root->d_inode->i_mutex, I_MUTEX_CHILD);
 		dentry = lookup_one_len(PRIVROOT_NAME, s->s_root,
 					strlen(PRIVROOT_NAME));
 		if (!IS_ERR(dentry)) {
@@ -993,6 +988,7 @@ int reiserfs_xattr_init(struct super_blo
 			}
 		} else
 			err = PTR_ERR(dentry);
+		mutex_unlock(&s->s_root->d_inode->i_mutex);
 
 		if (!err && dentry) {
 			s->s_root->d_op = &xattr_lookup_poison_ops;
-- 
Jeff Mahoney
SUSE Labs

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Alexander Beregalov]

2009/5/1 Jeff Mahoney <jeffm@suse.com>:
>  2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
>  bugs by ensuring that lookup_one_len is always called under i_mutex.
>
>  This patch expands the i_mutex locking to enclose lookup_one_len. This was
>  always required, but not not enforced in the reiserfs code since it
>  does locking around the xattr interactions with the xattr_sem.
>
>  This is obvious enough, but it survived an overnight 50 thread ACL test.
>
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
Tested-by: Alexander Beregalov <a.beregalov@gmail.com>

It works, thanks.

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Andrew Morton]

On Fri, 01 May 2009 12:11:12 -0400
Jeff Mahoney <jeffm@suse.com> wrote:

>  2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
>  bugs by ensuring that lookup_one_len is always called under i_mutex.
> 
>  This patch expands the i_mutex locking to enclose lookup_one_len. This was
>  always required, but not not enforced in the reiserfs code since it
>  does locking around the xattr interactions with the xattr_sem.

cool, so this will fix all those backtraces people have been reporting
coming out of the reiserfs xattr code lately?

>  This is obvious enough, but it survived an overnight 50 thread ACL test.

That sounds a bit pessimistic.  I think I'll s/but/and/ ;)

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Morton wrote:
> On Fri, 01 May 2009 12:11:12 -0400
> Jeff Mahoney <jeffm@suse.com> wrote:
> 
>>  2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
>>  bugs by ensuring that lookup_one_len is always called under i_mutex.
>>
>>  This patch expands the i_mutex locking to enclose lookup_one_len. This was
>>  always required, but not not enforced in the reiserfs code since it
>>  does locking around the xattr interactions with the xattr_sem.
> 
> cool, so this will fix all those backtraces people have been reporting
> coming out of the reiserfs xattr code lately?

Yes.

>>  This is obvious enough, but it survived an overnight 50 thread ACL test.
> 
> That sounds a bit pessimistic.  I think I'll s/but/and/ ;)

Yeah, that's more accurate. :)

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkn7XU4ACgkQLPWxlyuTD7JOAgCfcVV+kowmYbaBKNCZG7BfNx3V
9dwAoKGB7Uu2RkwXtkpaaCi2ADtQszC1
=B4is
-----END PGP SIGNATURE-----

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Al Viro]

On Fri, May 01, 2009 at 12:11:12PM -0400, Jeff Mahoney wrote:
>  2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
>  bugs by ensuring that lookup_one_len is always called under i_mutex.
> 
>  This patch expands the i_mutex locking to enclose lookup_one_len. This was
>  always required, but not not enforced in the reiserfs code since it
>  does locking around the xattr interactions with the xattr_sem.
> 
>  This is obvious enough, but it survived an overnight 50 thread ACL test.

It's not enough, unfortunately ;-/  It deals with the warning, but it
leaves an actual hole in there.

Look: what happens if we mount it r/o without that directory and then
remount r/w?  We get dentry for privroot, hash it (negative at that point),
then do actual mkdir, unlock root and modify the ->d_compare() of root
to reject lookups on that sucker.  Too late - in the meanwhile lookups
might very well come and find privroot in dcache.

BTW, the way ->d_compare() is done in there is rather dumb -
	if (q1 == &priv_root->d_name)
		return -ENOENT;
	...
would do just as well.  Why don't we do that lookup *once* (on ->get_sb(),
before anything can come and race with us), and then just keep negative
dentry if the directory hadn't been around?  And set d_compare() for root
immediately after that lookup...

I've applied your patch as-is, and unless you have objections to the
variant above I'll do that as incremental.  Comments?

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Al Viro]

On Sun, May 03, 2009 at 09:52:36AM +0100, Al Viro wrote:
> On Fri, May 01, 2009 at 12:11:12PM -0400, Jeff Mahoney wrote:
> >  2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
> >  bugs by ensuring that lookup_one_len is always called under i_mutex.
> > 
> >  This patch expands the i_mutex locking to enclose lookup_one_len. This was
> >  always required, but not not enforced in the reiserfs code since it
> >  does locking around the xattr interactions with the xattr_sem.
> > 
> >  This is obvious enough, but it survived an overnight 50 thread ACL test.
> 
> It's not enough, unfortunately ;-/  It deals with the warning, but it
> leaves an actual hole in there.
> 
> Look: what happens if we mount it r/o without that directory and then
> remount r/w?  We get dentry for privroot, hash it (negative at that point),
> then do actual mkdir, unlock root and modify the ->d_compare() of root
> to reject lookups on that sucker.  Too late - in the meanwhile lookups
> might very well come and find privroot in dcache.
> 
> BTW, the way ->d_compare() is done in there is rather dumb -
> 	if (q1 == &priv_root->d_name)
> 		return -ENOENT;
> 	...
> would do just as well.  Why don't we do that lookup *once* (on ->get_sb(),
> before anything can come and race with us), and then just keep negative
> dentry if the directory hadn't been around?  And set d_compare() for root
> immediately after that lookup...
> 
> I've applied your patch as-is, and unless you have objections to the
> variant above I'll do that as incremental.  Comments?

BTW, what in the name of everything unholy is ->xattr_root?  Never
assigned a non-NULL value...

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Al Viro]

On Sun, May 03, 2009 at 10:15:08AM +0100, Al Viro wrote:

> > I've applied your patch as-is, and unless you have objections to the
> > variant above I'll do that as incremental.  Comments?
> 
> BTW, what in the name of everything unholy is ->xattr_root?  Never
> assigned a non-NULL value...

Looks like your xattr journalling is b0rken - the check in there should
be that for ->priv_root, AFAICS.  Anyway, promised incremental follows (FWIW,
it's commit ff4559 in git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/
on #untested).

[PATCH] Always lookup priv_root on reiserfs mount and keep it

... even if it's a negative dentry.  That way we can set ->d_op on
root before anyone could race with us.  Simplify d_compare(), while
we are at it.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/reiserfs/super.c            |    6 ++-
 fs/reiserfs/xattr.c            |   86 +++++++++++++++++-----------------------
 include/linux/reiserfs_xattr.h |    1 +
 3 files changed, 41 insertions(+), 52 deletions(-)

diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 5d0064d..f45cac9 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -1845,7 +1845,8 @@ static int reiserfs_fill_super(struct super_block *s, void *data, int silent)
 			goto error;
 		}
 
-		if ((errval = reiserfs_xattr_init(s, s->s_flags))) {
+		if ((errval = reiserfs_lookup_privroot(s)) ||
+		    (errval = reiserfs_xattr_init(s, s->s_flags))) {
 			dput(s->s_root);
 			s->s_root = NULL;
 			goto error;
@@ -1858,7 +1859,8 @@ static int reiserfs_fill_super(struct super_block *s, void *data, int silent)
 			reiserfs_info(s, "using 3.5.x disk format\n");
 		}
 
-		if ((errval = reiserfs_xattr_init(s, s->s_flags))) {
+		if ((errval = reiserfs_lookup_privroot(s)) ||
+		    (errval = reiserfs_xattr_init(s, s->s_flags))) {
 			dput(s->s_root);
 			s->s_root = NULL;
 			goto error;
diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
index 31a3dbb..2891f78 100644
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -903,16 +903,19 @@ static int create_privroot(struct dentry *dentry)
 	WARN_ON_ONCE(!mutex_is_locked(&inode->i_mutex));
 
 	err = xattr_mkdir(inode, dentry, 0700);
-	if (err) {
-		dput(dentry);
-		dentry = NULL;
+	if (err || !dentry->d_inode) {
+		reiserfs_warning(dentry->d_sb, "jdm-20006",
+				 "xattrs/ACLs enabled and couldn't "
+				 "find/create .reiserfs_priv. "
+				 "Failing mount.");
+		return -EOPNOTSUPP;
 	}
 
-	if (dentry && dentry->d_inode)
-		reiserfs_info(dentry->d_sb, "Created %s - reserved for xattr "
-			      "storage.\n", PRIVROOT_NAME);
+	dentry->d_inode->i_flags |= S_PRIVATE;
+	reiserfs_info(dentry->d_sb, "Created %s - reserved for xattr "
+		      "storage.\n", PRIVROOT_NAME);
 
-	return err;
+	return 0;
 }
 
 static int xattr_mount_check(struct super_block *s)
@@ -944,11 +947,9 @@ static int
 xattr_lookup_poison(struct dentry *dentry, struct qstr *q1, struct qstr *name)
 {
 	struct dentry *priv_root = REISERFS_SB(dentry->d_sb)->priv_root;
-	if (name->len == priv_root->d_name.len &&
-	    name->hash == priv_root->d_name.hash &&
-	    !memcmp(name->name, priv_root->d_name.name, name->len)) {
+	if (container_of(q1, struct dentry, d_name) == priv_root)
 		return -ENOENT;
-	} else if (q1->len == name->len &&
+	if (q1->len == name->len &&
 		   !memcmp(q1->name, name->name, name->len))
 		return 0;
 	return 1;
@@ -958,6 +959,27 @@ static const struct dentry_operations xattr_lookup_poison_ops = {
 	.d_compare = xattr_lookup_poison,
 };
 
+int reiserfs_lookup_privroot(struct super_block *s)
+{
+	struct dentry *dentry;
+	int err = 0;
+
+	/* If we don't have the privroot located yet - go find it */
+	mutex_lock(&s->s_root->d_inode->i_mutex);
+	dentry = lookup_one_len(PRIVROOT_NAME, s->s_root,
+				strlen(PRIVROOT_NAME));
+	if (!IS_ERR(dentry)) {
+		REISERFS_SB(s)->priv_root = dentry;
+		s->s_root->d_op = &xattr_lookup_poison_ops;
+		if (dentry->d_inode)
+			dentry->d_inode->i_flags |= S_PRIVATE;
+	} else
+		err = PTR_ERR(dentry);
+	mutex_unlock(&s->s_root->d_inode->i_mutex);
+
+	return err;
+}
+
 /* We need to take a copy of the mount flags since things like
  * MS_RDONLY don't get set until *after* we're called.
  * mount_flags != mount_options */
@@ -969,48 +991,12 @@ int reiserfs_xattr_init(struct super_block *s, int mount_flags)
 	err = xattr_mount_check(s);
 	if (err)
 		goto error;
-#endif
 
-	/* If we don't have the privroot located yet - go find it */
-	if (!REISERFS_SB(s)->priv_root) {
-		struct dentry *dentry;
-		mutex_lock_nested(&s->s_root->d_inode->i_mutex, I_MUTEX_CHILD);
-		dentry = lookup_one_len(PRIVROOT_NAME, s->s_root,
-					strlen(PRIVROOT_NAME));
-		if (!IS_ERR(dentry)) {
-#ifdef CONFIG_REISERFS_FS_XATTR
-			if (!(mount_flags & MS_RDONLY) && !dentry->d_inode)
-				err = create_privroot(dentry);
-#endif
-			if (!dentry->d_inode) {
-				dput(dentry);
-				dentry = NULL;
-			}
-		} else
-			err = PTR_ERR(dentry);
+	if (!REISERFS_SB(s)->priv_root->d_inode && !(mount_flags & MS_RDONLY)) {
+		mutex_lock(&s->s_root->d_inode->i_mutex);
+		err = create_privroot(REISERFS_SB(s)->priv_root);
 		mutex_unlock(&s->s_root->d_inode->i_mutex);
-
-		if (!err && dentry) {
-			s->s_root->d_op = &xattr_lookup_poison_ops;
-			dentry->d_inode->i_flags |= S_PRIVATE;
-			REISERFS_SB(s)->priv_root = dentry;
-#ifdef CONFIG_REISERFS_FS_XATTR
-		/* xattrs are unavailable */
-		} else if (!(mount_flags & MS_RDONLY)) {
-			/* If we're read-only it just means that the dir
-			 * hasn't been created. Not an error -- just no
-			 * xattrs on the fs. We'll check again if we
-			 * go read-write */
-			reiserfs_warning(s, "jdm-20006",
-					 "xattrs/ACLs enabled and couldn't "
-					 "find/create .reiserfs_priv. "
-					 "Failing mount.");
-			err = -EOPNOTSUPP;
-#endif
-		}
 	}
-
-#ifdef CONFIG_REISERFS_FS_XATTR
 	if (!err)
 		s->s_xattr = reiserfs_xattr_handlers;
 
diff --git a/include/linux/reiserfs_xattr.h b/include/linux/reiserfs_xattr.h
index dcae01e..fea1a8e 100644
--- a/include/linux/reiserfs_xattr.h
+++ b/include/linux/reiserfs_xattr.h
@@ -38,6 +38,7 @@ struct nameidata;
 int reiserfs_xattr_register_handlers(void) __init;
 void reiserfs_xattr_unregister_handlers(void);
 int reiserfs_xattr_init(struct super_block *sb, int mount_flags);
+int reiserfs_lookup_privroot(struct super_block *sb);
 int reiserfs_delete_xattrs(struct inode *inode);
 int reiserfs_chown_xattrs(struct inode *inode, struct iattr *attrs);
 
-- 
1.5.6.5

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Al Viro wrote:
> On Sun, May 03, 2009 at 09:52:36AM +0100, Al Viro wrote:
>> On Fri, May 01, 2009 at 12:11:12PM -0400, Jeff Mahoney wrote:
>>>  2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
>>>  bugs by ensuring that lookup_one_len is always called under i_mutex.
>>>
>>>  This patch expands the i_mutex locking to enclose lookup_one_len. This was
>>>  always required, but not not enforced in the reiserfs code since it
>>>  does locking around the xattr interactions with the xattr_sem.
>>>
>>>  This is obvious enough, but it survived an overnight 50 thread ACL test.
>> It's not enough, unfortunately ;-/  It deals with the warning, but it
>> leaves an actual hole in there.
>>
>> Look: what happens if we mount it r/o without that directory and then
>> remount r/w?  We get dentry for privroot, hash it (negative at that point),
>> then do actual mkdir, unlock root and modify the ->d_compare() of root
>> to reject lookups on that sucker.  Too late - in the meanwhile lookups
>> might very well come and find privroot in dcache.
>>
>> BTW, the way ->d_compare() is done in there is rather dumb -
>> 	if (q1 == &priv_root->d_name)
>> 		return -ENOENT;
>> 	...
>> would do just as well.  Why don't we do that lookup *once* (on ->get_sb(),
>> before anything can come and race with us), and then just keep negative
>> dentry if the directory hadn't been around?  And set d_compare() for root
>> immediately after that lookup...
>>
>> I've applied your patch as-is, and unless you have objections to the
>> variant above I'll do that as incremental.  Comments?
> 
> BTW, what in the name of everything unholy is ->xattr_root?  Never
> assigned a non-NULL value...

Huh. I didn't see that still in there. That's an artifact of an earlier
version of the code where I kept a reference to /.reiserfs_priv/xattrs.
Then I decided that .reiserfs_priv was all I needed to cache (to avoid
recursive i_mutex locking on the fs root) and dropped the caching of
xattrs. Looks like it didn't get totally cleared out.

- -Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkn+dEgACgkQLPWxlyuTD7J4/ACggM4bSYzp8zuS8KXf2WaFSpS4
458An1YCcTf4hYXjFXuU8ZS2eEWJCpaa
=ZDpK
-----END PGP SIGNATURE-----

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Al Viro]

On Mon, May 04, 2009 at 12:51:20AM -0400, Jeff Mahoney wrote:

> Huh. I didn't see that still in there. That's an artifact of an earlier
> version of the code where I kept a reference to /.reiserfs_priv/xattrs.
> Then I decided that .reiserfs_priv was all I needed to cache (to avoid
> recursive i_mutex locking on the fs root) and dropped the caching of
> xattrs. Looks like it didn't get totally cleared out.

It's not that simple ;-/  You check it in journalling code, AFAICS in order
to decide how much will that sucker take (due to extra mkdir?) and something
will need to be done with that check.

Anyway, I'm going to push all that stuff to #for-next, so that -next would
pick it.  I have *not* touched the xattr_root logics, so if you could do
that on top of your patch + my incremental...

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Al Viro wrote:
> On Mon, May 04, 2009 at 12:51:20AM -0400, Jeff Mahoney wrote:
> 
>> Huh. I didn't see that still in there. That's an artifact of an earlier
>> version of the code where I kept a reference to /.reiserfs_priv/xattrs.
>> Then I decided that .reiserfs_priv was all I needed to cache (to avoid
>> recursive i_mutex locking on the fs root) and dropped the caching of
>> xattrs. Looks like it didn't get totally cleared out.
> 
> It's not that simple ;-/  You check it in journalling code, AFAICS in order
> to decide how much will that sucker take (due to extra mkdir?) and something
> will need to be done with that check.

Yes, it's for an extra mkdir. Now that I've gotten some sleep and looked
at the code again, I see what you're saying. At least it's broken in a
performance way instead of causing a system crash or data corruption.

> Anyway, I'm going to push all that stuff to #for-next, so that -next would
> pick it.  I have *not* touched the xattr_root logics, so if you could do
> that on top of your patch + my incremental...

Ok, I'll fix that up and get some testing in.

- -Jeff


- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkn/Gn4ACgkQLPWxlyuTD7KclACeLNAOawW5fDxc2CqGZz4NUXpY
21AAoJdsK3EEPweBCuv1131j3Lm8x3lk
=0Iqb
-----END PGP SIGNATURE-----

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Jeff Mahoney]

Jeff Mahoney wrote:
> Al Viro wrote:
>> On Mon, May 04, 2009 at 12:51:20AM -0400, Jeff Mahoney wrote:
> 
>>> Huh. I didn't see that still in there. That's an artifact of an earlier
>>> version of the code where I kept a reference to /.reiserfs_priv/xattrs.
>>> Then I decided that .reiserfs_priv was all I needed to cache (to avoid
>>> recursive i_mutex locking on the fs root) and dropped the caching of
>>> xattrs. Looks like it didn't get totally cleared out.
>> It's not that simple ;-/  You check it in journalling code, AFAICS in order
>> to decide how much will that sucker take (due to extra mkdir?) and something
>> will need to be done with that check.
> 
> Yes, it's for an extra mkdir. Now that I've gotten some sleep and looked
> at the code again, I see what you're saying. At least it's broken in a
> performance way instead of causing a system crash or data corruption.
> 
>> Anyway, I'm going to push all that stuff to #for-next, so that -next would
>> pick it.  I have *not* touched the xattr_root logics, so if you could do
>> that on top of your patch + my incremental...
> 
> Ok, I'll fix that up and get some testing in.

Here's the patch to fix up the xattr root caching. I have two more that do some
cleanups that I'll post separately. The early load of the privroot means we can
get rid of some of the mess with hiding the entry during readdir and lookup.

I'll send all three as a series, separately as well.

-Jeff

---

 The xattr_root caching was broken from my previous patch set. It wouldn't
 cause corruption, but could cause decreased performance due to allocating
 a larger chunk of the journal (~ 27 blocks) than it would actually use.

 This patch loads the xattr root dentry at xattr initialization and creates
 it on-demand. Since we're using the cached dentry, there's no point
 in keeping lookup_or_create_dir around, so that's removed.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/reiserfs/xattr.c            |   73 +++++++++++++++++++++++++----------------
 include/linux/reiserfs_fs_sb.h |    2 -
 include/linux/reiserfs_xattr.h |    2 -
 3 files changed, 48 insertions(+), 29 deletions(-)

--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -113,36 +113,28 @@ static int xattr_rmdir(struct inode *dir
 
 #define xattr_may_create(flags)	(!flags || flags & XATTR_CREATE)
 
-/* Returns and possibly creates the xattr dir. */
-static struct dentry *lookup_or_create_dir(struct dentry *parent,
-					    const char *name, int flags)
+static struct dentry *open_xa_root(struct super_block *sb, int flags)
 {
-	struct dentry *dentry;
-	BUG_ON(!parent);
+	struct dentry *privroot = REISERFS_SB(sb)->priv_root;
+	struct dentry *xaroot;
+	if (!privroot->d_inode)
+		return ERR_PTR(-ENODATA);
 
-	mutex_lock_nested(&parent->d_inode->i_mutex, I_MUTEX_XATTR);
-	dentry = lookup_one_len(name, parent, strlen(name));
-	if (!IS_ERR(dentry) && !dentry->d_inode) {
-		int err = -ENODATA;
+	mutex_lock_nested(&privroot->d_inode->i_mutex, I_MUTEX_XATTR);
 
+	xaroot = dget(REISERFS_SB(sb)->xattr_root);
+	if (!xaroot->d_inode) {
+		int err = -ENODATA;
 		if (xattr_may_create(flags))
-			err = xattr_mkdir(parent->d_inode, dentry, 0700);
-
+			err = xattr_mkdir(privroot->d_inode, xaroot, 0700);
 		if (err) {
-			dput(dentry);
-			dentry = ERR_PTR(err);
+			dput(xaroot);
+			xaroot = ERR_PTR(err);
 		}
 	}
-	mutex_unlock(&parent->d_inode->i_mutex);
-	return dentry;
-}
 
-static struct dentry *open_xa_root(struct super_block *sb, int flags)
-{
-	struct dentry *privroot = REISERFS_SB(sb)->priv_root;
-	if (!privroot)
-		return ERR_PTR(-ENODATA);
-	return lookup_or_create_dir(privroot, XAROOT_NAME, flags);
+	mutex_unlock(&privroot->d_inode->i_mutex);
+	return xaroot;
 }
 
 static struct dentry *open_xa_dir(const struct inode *inode, int flags)
@@ -158,10 +150,22 @@ static struct dentry *open_xa_dir(const
 		 le32_to_cpu(INODE_PKEY(inode)->k_objectid),
 		 inode->i_generation);
 
-	xadir = lookup_or_create_dir(xaroot, namebuf, flags);
+	mutex_lock_nested(&xaroot->d_inode->i_mutex, I_MUTEX_XATTR);
+
+	xadir = lookup_one_len(namebuf, xaroot, strlen(namebuf));
+	if (!IS_ERR(xadir) && !xadir->d_inode) {
+		int err = -ENODATA;
+		if (xattr_may_create(flags))
+			err = xattr_mkdir(xaroot->d_inode, xadir, 0700);
+		if (err) {
+			dput(xadir);
+			xadir = ERR_PTR(err);
+		}
+	}
+
+	mutex_unlock(&xaroot->d_inode->i_mutex);
 	dput(xaroot);
 	return xadir;
-
 }
 
 /* The following are side effects of other operations that aren't explicitly
@@ -986,19 +990,33 @@ int reiserfs_lookup_privroot(struct supe
 int reiserfs_xattr_init(struct super_block *s, int mount_flags)
 {
 	int err = 0;
+	struct dentry *privroot = REISERFS_SB(s)->priv_root;
 
 #ifdef CONFIG_REISERFS_FS_XATTR
 	err = xattr_mount_check(s);
 	if (err)
 		goto error;

-	if (!REISERFS_SB(s)->priv_root->d_inode && !(mount_flags & MS_RDONLY)) {
+	if (!privroot->d_inode && !(mount_flags & MS_RDONLY)) {
 		mutex_lock(&s->s_root->d_inode->i_mutex);
 		err = create_privroot(REISERFS_SB(s)->priv_root);
 		mutex_unlock(&s->s_root->d_inode->i_mutex);
 	}
-	if (!err)
+
+	if (privroot->d_inode) {
 		s->s_xattr = reiserfs_xattr_handlers;
+		mutex_lock(&privroot->d_inode->i_mutex);
+		if (!REISERFS_SB(s)->xattr_root) {
+			struct dentry *dentry;
+			dentry = lookup_one_len(XAROOT_NAME, privroot,
+						strlen(XAROOT_NAME));
+			if (!IS_ERR(dentry))
+				REISERFS_SB(s)->xattr_root = dentry;
+			else
+				err = PTR_ERR(dentry);
+		}
+		mutex_unlock(&privroot->d_inode->i_mutex);
+	}
 
 error:
 	if (err) {
@@ -1008,11 +1026,12 @@ error:
 #endif
 
 	/* The super_block MS_POSIXACL must mirror the (no)acl mount option. */
-	s->s_flags = s->s_flags & ~MS_POSIXACL;
 #ifdef CONFIG_REISERFS_FS_POSIX_ACL
 	if (reiserfs_posixacl(s))
 		s->s_flags |= MS_POSIXACL;
+	else
 #endif
+		s->s_flags &= ~MS_POSIXACL;
 
 	return err;
 }
--- a/include/linux/reiserfs_fs_sb.h
+++ b/include/linux/reiserfs_fs_sb.h
@@ -402,7 +402,7 @@ struct reiserfs_sb_info {
 	int reserved_blocks;	/* amount of blocks reserved for further allocations */
 	spinlock_t bitmap_lock;	/* this lock on now only used to protect reserved_blocks variable */
 	struct dentry *priv_root;	/* root of /.reiserfs_priv */
-	struct dentry *xattr_root;	/* root of /.reiserfs_priv/.xa */
+	struct dentry *xattr_root;	/* root of /.reiserfs_priv/xattrs */
 	int j_errno;
 #ifdef CONFIG_QUOTA
 	char *s_qf_names[MAXQUOTAS];
--- a/include/linux/reiserfs_xattr.h
+++ b/include/linux/reiserfs_xattr.h
@@ -98,7 +98,7 @@ static inline size_t reiserfs_xattr_jcre
 
 	if ((REISERFS_I(inode)->i_flags & i_has_xattr_dir) == 0) {
 		nblocks += JOURNAL_BLOCKS_PER_OBJECT(inode->i_sb);
-		if (REISERFS_SB(inode->i_sb)->xattr_root == NULL)
+		if (!REISERFS_SB(inode->i_sb)->xattr_root->d_inode)
 			nblocks += JOURNAL_BLOCKS_PER_OBJECT(inode->i_sb);
 	}
 
-- 
Jeff Mahoney
SUSE Labs

Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Al Viro wrote:
> On Fri, May 01, 2009 at 12:11:12PM -0400, Jeff Mahoney wrote:
>>  2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
>>  bugs by ensuring that lookup_one_len is always called under i_mutex.
>>
>>  This patch expands the i_mutex locking to enclose lookup_one_len. This was
>>  always required, but not not enforced in the reiserfs code since it
>>  does locking around the xattr interactions with the xattr_sem.
>>
>>  This is obvious enough, but it survived an overnight 50 thread ACL test.
> 
> It's not enough, unfortunately ;-/  It deals with the warning, but it
> leaves an actual hole in there.
> 
> Look: what happens if we mount it r/o without that directory and then
> remount r/w?  We get dentry for privroot, hash it (negative at that point),
> then do actual mkdir, unlock root and modify the ->d_compare() of root
> to reject lookups on that sucker.  Too late - in the meanwhile lookups
> might very well come and find privroot in dcache.
> 
> BTW, the way ->d_compare() is done in there is rather dumb -
> 	if (q1 == &priv_root->d_name)
> 		return -ENOENT;
> 	...
> would do just as well.  Why don't we do that lookup *once* (on ->get_sb(),
> before anything can come and race with us), and then just keep negative
> dentry if the directory hadn't been around?  And set d_compare() for root
> immediately after that lookup...
> 
> I've applied your patch as-is, and unless you have objections to the
> variant above I'll do that as incremental.  Comments?

Of course you're right about the lookup hole.

The lookup during remount is from when the code didn't enable xattrs
unconditionally for security and trusted attributes. The privroot would
only be created when mounted read-write with -oacl and/or -ouser_xattr.
Now xattrs are enabled uncondtionally, so any read-write mount will
create that if xattrs are enabled in the kernel. It used to avoid
caching a negative lookup that would never get used.

I don't have any objections to any of your suggestions.

Thanks!

- -Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkn+dp0ACgkQLPWxlyuTD7KuwQCgmrYQhuDy+HylXPL46Yb2R9Y+
Fr0AoIKRyL4mX1wCR+R9WN74zULTrbXT
=LSDi
-----END PGP SIGNATURE-----