[PATCH] btrfs: Silence a static checker locking warning

[PATCH] btrfs: Silence a static checker locking warning

[Dan Carpenter]

Back in the day, before commit 0b246afa62b0 ("btrfs: root->fs_info
cleanup, add fs_info convenience variables") then we used to take
different locks.  But now it's just one lock and the static checkers
think we can call down_read(&fs_info->subvol_sem); twice in a row which
would lead to a deadlock.

That code is several years old now so presumably both (old_ino ==
BTRFS_FIRST_FREE_OBJECTID) and (new_ino == BTRFS_FIRST_FREE_OBJECTID)
conditions can't be true at the same time or the bug would have showed
up in testing.  I have re-written the code though to make it cleaner and
to silence the static checkers.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/btrfs/inode.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9b0e3e2d589c..039a12f51cd7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9423,9 +9423,8 @@ static int btrfs_rename_exchange(struct inode *old_dir,
 	btrfs_init_log_ctx(&ctx_dest, new_inode);
 
 	/* close the race window with snapshot create/destroy ioctl */
-	if (old_ino == BTRFS_FIRST_FREE_OBJECTID)
-		down_read(&fs_info->subvol_sem);
-	if (new_ino == BTRFS_FIRST_FREE_OBJECTID)
+	if (old_ino == BTRFS_FIRST_FREE_OBJECTID ||
+	    new_ino == BTRFS_FIRST_FREE_OBJECTID)
 		down_read(&fs_info->subvol_sem);
 
 	/*
@@ -9644,9 +9643,8 @@ static int btrfs_rename_exchange(struct inode *old_dir,
 		ret = ret ? ret : ret2;
 	}
 out_notrans:
-	if (new_ino == BTRFS_FIRST_FREE_OBJECTID)
-		up_read(&fs_info->subvol_sem);
-	if (old_ino == BTRFS_FIRST_FREE_OBJECTID)
+	if (new_ino == BTRFS_FIRST_FREE_OBJECTID ||
+	    old_ino == BTRFS_FIRST_FREE_OBJECTID)
 		up_read(&fs_info->subvol_sem);
 
 	return ret;
-- 
2.17.1

Re: [PATCH] btrfs: Silence a static checker locking warning

[David Sterba]

On Sat, Feb 09, 2019 at 12:02:55PM +0300, Dan Carpenter wrote:
> Back in the day, before commit 0b246afa62b0 ("btrfs: root->fs_info
> cleanup, add fs_info convenience variables") then we used to take
> different locks.

Nope, it's the same per-filesystem lock, just the old code got there
in two different ways (ie. two subvolume roots).

> But now it's just one lock and the static checkers
> think we can call down_read(&fs_info->subvol_sem); twice in a row which
> would lead to a deadlock.

Why? It's read side of a semaphore.

> That code is several years old now so presumably both (old_ino ==
> BTRFS_FIRST_FREE_OBJECTID) and (new_ino == BTRFS_FIRST_FREE_OBJECTID)
> conditions can't be true at the same time or the bug would have showed
> up in testing.

Why do you think it's a bug? If you are sure that there's a bug we've
overlooked, please state it in the changelog, the rationale you've
provided is very vague.

And I believe also wrong. The rename-exchange cannot work between two
subvolumes, but we still can cross-rename two subvolumes. In this
example hierarchy:

/
- subvol1 (inode number 256, ie. BTRFS_FIRST_FREE_OBJECTID)
  - file1
- subvol2 (inode number 256, ie. BTRFS_FIRST_FREE_OBJECTID)
  - file2

btrfs_rename_exchange leads to this:

/
- subvol1
  - file2
- subvol2
  - file1

There's no common tool that supports renameat2, so I'm using the one
from fstests/src/renameat2.c to verify that, and it does indeed work as
expected.

> I have re-written the code though to make it cleaner and
> to silence the static checkers.

Maybe there's something new the static checker needs to learn.

Re: [PATCH] btrfs: Silence a static checker locking warning

[David Sterba]

On Mon, Feb 11, 2019 at 05:36:13PM +0100, David Sterba wrote:
> On Sat, Feb 09, 2019 at 12:02:55PM +0300, Dan Carpenter wrote:
> > Back in the day, before commit 0b246afa62b0 ("btrfs: root->fs_info
> > cleanup, add fs_info convenience variables") then we used to take
> > different locks.
> 
> Nope, it's the same per-filesystem lock, just the old code got there
> in two different ways (ie. two subvolume roots).
> 
> > But now it's just one lock and the static checkers
> > think we can call down_read(&fs_info->subvol_sem); twice in a row which
> > would lead to a deadlock.
> 
> Why? It's read side of a semaphore.
> 
> > That code is several years old now so presumably both (old_ino ==
> > BTRFS_FIRST_FREE_OBJECTID) and (new_ino == BTRFS_FIRST_FREE_OBJECTID)
> > conditions can't be true at the same time or the bug would have showed
> > up in testing.
> 
> Why do you think it's a bug? If you are sure that there's a bug we've
> overlooked, please state it in the changelog, the rationale you've
> provided is very vague.
> 
> And I believe also wrong. The rename-exchange cannot work between two
> subvolumes, but we still can cross-rename two subvolumes. In this
> example hierarchy:
> 
> /
> - subvol1 (inode number 256, ie. BTRFS_FIRST_FREE_OBJECTID)
>   - file1
> - subvol2 (inode number 256, ie. BTRFS_FIRST_FREE_OBJECTID)
>   - file2
> 
> btrfs_rename_exchange leads to this:
> 
> /
> - subvol1
>   - file2
> - subvol2
>   - file1
> 
> There's no common tool that supports renameat2, so I'm using the one
> from fstests/src/renameat2.c to verify that, and it does indeed work as
> expected.

Lockdep was forgiving and did not deadlock, that I would notice while
running the test. There's a warning in the log about the recursive
locking. So we need to add the lock annotation or merge them to a single
location as you suggest.

And also add a test to fstests, as the subvolume related testcases are
completely missing.

Re: [PATCH] btrfs: Silence a static checker locking warning

[Dan Carpenter]

On Mon, Feb 11, 2019 at 05:36:13PM +0100, David Sterba wrote:
> > I have re-written the code though to make it cleaner and
> > to silence the static checkers.
> 
> Maybe there's something new the static checker needs to learn.

Gar.  Yes.  You're right.  I hadn't thought about that read locks could
nest.

regards,
dan carpenter