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

*Forwarded Conversation*
Subject: *[2.6.20.4] BUG: dentry xattrs still in use in shrink_dcache_for_umount() with reiserfs*
------------------------

* From: Andrea Righi* <[email protected]> Reply-To:
[email protected]
To: [email protected]
Cc: LKML <[email protected]>, Andrew Morton < [email protected]>
Date: Fri, Apr 13, 2007 at 3:04 PM
Attachments: config.gz

I can reproduce the problem umounting my /var (reiserfs), but it doesn't occur with /usr or /opt, that are reiserfs too.

It seems very similar to this issue:
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc3/2.6.18-rc3-mm2/hot-fixes/reiserfs-make-sure-all-dentries-refs-are-released-before-calling-kill_block_super-try-2.patch

How the xattrs->d_count can be 1 if the dentry is explicitly released in reiserfs_kill_sb(), before calling kill_super_block()?

(config attached)

-Andrea

BUG: Dentry dfcd2570{i=21bc,n=xattrs} still in use (1) [unmount of reiserfs

dm-4]

Hello

Quoted 13 lines · ReiserFS Developers Mailing List

*Forwarded Conversation*
Subject: *[2.6.20.4] BUG: dentry xattrs still in use in shrink_dcache_for_umount() with reiserfs*
------------------------

* From: Andrea Righi* <[email protected]> Reply-To:
[email protected]
To: [email protected]
Cc: LKML <[email protected]>, Andrew Morton < [email protected]>
Date: Fri, Apr 13, 2007 at 3:04 PM
Attachments: config.gz

Quoted 9 lines

I can reproduce the problem umounting my /var (reiserfs), but it doesn't occur with /usr or /opt, that are reiserfs too.

It seems very similar to this issue:
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc3/2.6.18-rc3-mm2/hot-fixes/reiserfs-make-sure-all-dentries-refs-are-released-before-calling-kill_block_super-try-2.patch

How the xattrs->d_count can be 1 if the dentry is explicitly released in reiserfs_kill_sb(), before calling kill_super_block()?

Jeff, in 2.6.20/fs/reiserfs/xattr.c we have get_xa_root, which may increment xattr_root reference count either by 1 or 2:

If REISERFS_SB()->xattr_root != NULL - dentry reference count is incremented by 1 with dget.

If REISERFS_SB()->xattr_root == NULL, __get_xa_root is called which may increase xattr_root dentry reference count by 2:
once in lookup_one_len->__lookup_hash->cached_lookup->__d_lookup and once explicitly with dget in __get_xa_root.

Do you think that could be a reason of the extra reference count on xattr_root dentry?

Quoted 1 line

Do you think that could be a reason of the extra reference count on xattr_root dentry?

No, I don't think it is. Looking at the code now, it seems obvious, but I didn't notice it before and nobody else has reported a problem.

getxattr() doesn't require any VFS locking. When we get down into the reiserfs code, it takes a read lock. If we get two concurrent threads looking up an xattr before the root has been saved, there's a window where REISERFS_SB(s)->xattr_root is NULL but we've already looked it up and taken a reference on it.

I have a patch set to clean up the extended attribute code that fixes this problem along the way by killing off the xattr locks and using the backing files/dirs i_mutex instead. I'll post them to the reiserfs mailing list.

Quoted 10 lines · On Wed, 18 Apr 2007 11:00:00 -0400 Jeff Mahoney <[email protected]> wrote:

Quoted 1 line

Do you think that could be a reason of the extra reference count on xattr_root dentry?

No, I don't think it is. Looking at the code now, it seems obvious, but I didn't notice it before and nobody else has reported a problem.

getxattr() doesn't require any VFS locking. When we get down into the reiserfs code, it takes a read lock. If we get two concurrent threads looking up an xattr before the root has been saved, there's a window where REISERFS_SB(s)->xattr_root is NULL but we've already looked it up and taken a reference on it.

Quoted 4 lines

I have a patch set to clean up the extended attribute code that fixes this problem along the way by killing off the xattr locks and using the backing files/dirs i_mutex instead. I'll post them to the reiserfs mailing list.

Do we have anything suitable for 2.6.21 which will address this crash?

Also, it's not clear to me how many users we can expect to be impacted by it. I assume that if the same bug is in 2.6.20 then the answer is "not many". How come Andrea is able to keep hitting it?

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.

Quoted 14 lines · Jeff Mahoney

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.

Quoted 4 lines

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?

Quoted 19 lines · Andrea Righi

Quoted 14 lines · Jeff Mahoney

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.

Quoted 4 lines

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.

Quoted 6 lines

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?

Your patch fixes the problem. That'll teach me to spin a patch up while I'm headed out the door. I think a cleaner solution for now is just to refactor get_xa_root, __get_xa_root, and create_xa_root into one function to simplify the lookup and the locking. I think I'll add another step to my patch set that removes the caching of xattr_root entirely or creates in on mount, as the priv root is created. The privroot must be cached to avoid a deadlock on the fs-root directory's i_mutex, but xattr_root was cached as an optimzation. If we need to take privroot->i_mutex every time, there's no point in caching it since it will probably be in the dcache anyway.

-Jeff

The listxattr() and getxattr() operations are only protected by a read lock. As a result, if either of these operations run in parallel, a race condition exists where the xattr_root will end up being cached twice, which results in the leaking of a reference and a BUG() on umount.

This patch refactors get_xa_root(), __get_xa_root(), and create_xa_root(), into one get_xa_root() function that takes the appropriate locking around the entire critical section.

Quoted 8 lines · On Sat, 21 Apr 2007 11:26:31 -0400 Jeff Mahoney <[email protected]> wrote:

The listxattr() and getxattr() operations are only protected by a read lock. As a result, if either of these operations run in parallel, a race condition exists where the xattr_root will end up being cached twice, which results in the leaking of a reference and a BUG() on umount.

This patch refactors get_xa_root(), __get_xa_root(), and create_xa_root(), into one get_xa_root() function that takes the appropriate locking around the entire critical section.

Great, thanks.

Now we need to work out the timing. Our options are to shove it into 2.6.21 immediately, or to give it a run in 2.6.22-rc1 then backport into 2.6.21.x.

What is everyone's confidence level?

Quoted 8 lines · On Sat, 21 Apr 2007 11:26:31 -0400 Jeff Mahoney <[email protected]> wrote:

The listxattr() and getxattr() operations are only protected by a read lock. As a result, if either of these operations run in parallel, a race condition exists where the xattr_root will end up being cached twice, which results in the leaking of a reference and a BUG() on umount.

This patch refactors get_xa_root(), __get_xa_root(), and create_xa_root(), into one get_xa_root() function that takes the appropriate locking around the entire critical section.

Great, thanks.

Now we need to work out the timing. Our options are to shove it into 2.6.21 immediately, or to give it a run in 2.6.22-rc1 then backport into 2.6.21.x.

What is everyone's confidence level?