[PATCH][v3.2 stable tree] dcache: Balance rcu_read_lock in have_submounts()

[PATCH][v3.2 stable tree] dcache: Balance rcu_read_lock in have_submounts()

[Steven Rostedt]

Porting -rt to the latest 3.2 stable tree I triggered this bug:

=====================================
[ BUG: bad unlock balance detected! ]
-------------------------------------
rm/1638 is trying to release lock (rcu_read_lock) at:
[<c04fde6c>] rcu_read_unlock+0x0/0x23
but there are no more locks to release!

other info that might help us debug this:
2 locks held by rm/1638:
 #0:  (&sb->s_type->i_mutex_key#9/1){+.+.+.}, at: [<c04f93eb>] do_rmdir+0x5f/0xd2
 #1:  (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<c04f9329>] vfs_rmdir+0x49/0xac

stack backtrace:
Pid: 1638, comm: rm Not tainted 3.2.66-test-rt96+ #2
Call Trace:
 [<c083f390>] ? printk+0x1d/0x1f
 [<c0463cdf>] print_unlock_inbalance_bug+0xc3/0xcd
 [<c04653a8>] lock_release_non_nested+0x98/0x1ec
 [<c046228d>] ? trace_hardirqs_off_caller+0x18/0x90
 [<c0456f1c>] ? local_clock+0x2d/0x50
 [<c04fde6c>] ? d_hash+0x2f/0x2f
 [<c04fde6c>] ? d_hash+0x2f/0x2f
 [<c046568e>] lock_release+0x192/0x1ad
 [<c04fde83>] rcu_read_unlock+0x17/0x23
 [<c04ff344>] shrink_dcache_parent+0x227/0x270
 [<c04f9348>] vfs_rmdir+0x68/0xac
 [<c04f9424>] do_rmdir+0x98/0xd2
 [<c04f03ad>] ? fput+0x1a3/0x1ab
 [<c084dd42>] ? sysenter_exit+0xf/0x1a
 [<c0465b58>] ? trace_hardirqs_on_caller+0x118/0x149
 [<c04fa3e0>] sys_unlinkat+0x2b/0x35
 [<c084dd13>] sysenter_do_call+0x12/0x12




There's a path to calling rcu_read_unlock() without calling
rcu_read_lock() in have_submounts().

	goto positive;

positive:
	if (!locked && read_seqretry(&rename_lock, seq))
		goto rename_retry;

rename_retry:
	rcu_read_unlock();

in the above path, rcu_read_lock() is never done before calling
rcu_read_unlock();

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 fs/dcache.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 98cc4508e140..0d0ba5b2a8ae 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1035,8 +1035,10 @@ ascend:
 		write_sequnlock(&rename_lock);
 	return 0; /* No mount points found in tree */
 positive:
-	if (!locked && read_seqretry(&rename_lock, seq))
+	if (!locked && read_seqretry(&rename_lock, seq)) {
+		rcu_read_lock();
 		goto rename_retry;
+	}
 	if (locked)
 		write_sequnlock(&rename_lock);
 	return 1;
-- 
2.1.4

Re: [PATCH][v3.2 stable tree] dcache: Balance rcu_read_lock in have_submounts()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3009 bytes --]

On Sat, 2015-01-17 at 10:38 -0500, Steven Rostedt wrote:
> Porting -rt to the latest 3.2 stable tree I triggered this bug:
> 
> =====================================
> [ BUG: bad unlock balance detected! ]
> -------------------------------------
> rm/1638 is trying to release lock (rcu_read_lock) at:
> [<c04fde6c>] rcu_read_unlock+0x0/0x23
> but there are no more locks to release!
> 
> other info that might help us debug this:
> 2 locks held by rm/1638:
>  #0:  (&sb->s_type->i_mutex_key#9/1){+.+.+.}, at: [<c04f93eb>] do_rmdir+0x5f/0xd2
>  #1:  (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<c04f9329>] vfs_rmdir+0x49/0xac
> 
> stack backtrace:
> Pid: 1638, comm: rm Not tainted 3.2.66-test-rt96+ #2
> Call Trace:
>  [<c083f390>] ? printk+0x1d/0x1f
>  [<c0463cdf>] print_unlock_inbalance_bug+0xc3/0xcd
>  [<c04653a8>] lock_release_non_nested+0x98/0x1ec
>  [<c046228d>] ? trace_hardirqs_off_caller+0x18/0x90
>  [<c0456f1c>] ? local_clock+0x2d/0x50
>  [<c04fde6c>] ? d_hash+0x2f/0x2f
>  [<c04fde6c>] ? d_hash+0x2f/0x2f
>  [<c046568e>] lock_release+0x192/0x1ad
>  [<c04fde83>] rcu_read_unlock+0x17/0x23
>  [<c04ff344>] shrink_dcache_parent+0x227/0x270
>  [<c04f9348>] vfs_rmdir+0x68/0xac
>  [<c04f9424>] do_rmdir+0x98/0xd2
>  [<c04f03ad>] ? fput+0x1a3/0x1ab
>  [<c084dd42>] ? sysenter_exit+0xf/0x1a
>  [<c0465b58>] ? trace_hardirqs_on_caller+0x118/0x149
>  [<c04fa3e0>] sys_unlinkat+0x2b/0x35
>  [<c084dd13>] sysenter_do_call+0x12/0x12
> 
> 
> 
> 
> There's a path to calling rcu_read_unlock() without calling
> rcu_read_lock() in have_submounts().
> 
> 	goto positive;
> 
> positive:
> 	if (!locked && read_seqretry(&rename_lock, seq))
> 		goto rename_retry;
> 
> rename_retry:
> 	rcu_read_unlock();
> 
> in the above path, rcu_read_lock() is never done before calling
> rcu_read_unlock();

I've reviewed locking contexts in all three functions that I changed
when backporting "deal with deadlock in d_walk()".  It's actually worse
than you say:

- We don't hold this_parent->d_lock at the 'positive' label in
have_submounts(), but it is unlocked after 'rename_retry'.
- There is an rcu_read_unlock() after the 'out' label in
select_parent(), but it's not held at the 'goto out'.

Does the following patch work for you?

Ben.

--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1035,7 +1035,7 @@ ascend:
 	return 0; /* No mount points found in tree */
 positive:
 	if (!locked && read_seqretry(&rename_lock, seq))
-		goto rename_retry;
+		goto rename_retry_unlocked;
 	if (locked)
 		write_sequnlock(&rename_lock);
 	return 1;
@@ -1045,6 +1045,7 @@ rename_retry:
 	rcu_read_unlock();
 	if (locked)
 		goto again;
+rename_retry_unlocked:
 	locked = 1;
 	write_seqlock(&rename_lock);
 	goto again;
@@ -1109,6 +1110,7 @@ resume:
 		 */
 		if (found && need_resched()) {
 			spin_unlock(&dentry->d_lock);
+			rcu_read_lock();
 			goto out;
 		}
 
--- END ---

-- 
Ben Hutchings
When in doubt, use brute force. - Ken Thompson

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH][v3.2 stable tree] dcache: Balance rcu_read_lock in have_submounts()

[Steven Rostedt]

On Wed, 11 Feb 2015 03:15:37 +0000
Ben Hutchings <ben@decadent.org.uk> wrote:

> I've reviewed locking contexts in all three functions that I changed
> when backporting "deal with deadlock in d_walk()".  It's actually worse
> than you say:
> 
> - We don't hold this_parent->d_lock at the 'positive' label in
> have_submounts(), but it is unlocked after 'rename_retry'.
> - There is an rcu_read_unlock() after the 'out' label in
> select_parent(), but it's not held at the 'goto out'.
> 
> Does the following patch work for you?

I booted the original 3.2 kernel 3 times and it succeeded once, and
crashed the other two.

I applied this patch and booted it 5 times, and it succeeded every time.

I removed the patch, and it crashed on the very next boot.

Thus, I'd say it seems to have fixed the issue for me.

Tested-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> 
> Ben.
> 
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1035,7 +1035,7 @@ ascend:
>  	return 0; /* No mount points found in tree */
>  positive:
>  	if (!locked && read_seqretry(&rename_lock, seq))
> -		goto rename_retry;
> +		goto rename_retry_unlocked;
>  	if (locked)
>  		write_sequnlock(&rename_lock);
>  	return 1;
> @@ -1045,6 +1045,7 @@ rename_retry:
>  	rcu_read_unlock();
>  	if (locked)
>  		goto again;
> +rename_retry_unlocked:
>  	locked = 1;
>  	write_seqlock(&rename_lock);
>  	goto again;
> @@ -1109,6 +1110,7 @@ resume:
>  		 */
>  		if (found && need_resched()) {
>  			spin_unlock(&dentry->d_lock);
> +			rcu_read_lock();
>  			goto out;
>  		}
>  
> --- END ---
> 

Re: [PATCH][v3.2 stable tree] dcache: Balance rcu_read_lock in have_submounts()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]

On Thu, 2015-02-12 at 00:28 -0500, Steven Rostedt wrote:
> On Wed, 11 Feb 2015 03:15:37 +0000
> Ben Hutchings <ben@decadent.org.uk> wrote:
> 
> > I've reviewed locking contexts in all three functions that I changed
> > when backporting "deal with deadlock in d_walk()".  It's actually worse
> > than you say:
> > 
> > - We don't hold this_parent->d_lock at the 'positive' label in
> > have_submounts(), but it is unlocked after 'rename_retry'.
> > - There is an rcu_read_unlock() after the 'out' label in
> > select_parent(), but it's not held at the 'goto out'.
> > 
> > Does the following patch work for you?
> 
> I booted the original 3.2 kernel 3 times and it succeeded once, and
> crashed the other two.
> 
> I applied this patch and booted it 5 times, and it succeeded every time.
> 
> I removed the patch, and it crashed on the very next boot.
> 
> Thus, I'd say it seems to have fixed the issue for me.
> 
> Tested-by: Steven Rostedt <rostedt@goodmis.org>
[...]

Thanks.

Ben.

-- 
Ben Hutchings
Never attribute to conspiracy what can adequately be explained by stupidity.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]