[PATCH 1/2] btrfs: overwrite a leftover balance item instead of BUG_ON()

[Siddharth Mishra <smish149@asu.edu>]

[BUG]
A balance ioctl can hit this BUG_ON():

  kernel BUG at fs/btrfs/volumes.c:5088!
  Oops: invalid opcode: 0000 [#1] SMP NOPTI
  RIP: 0010:btrfs_balance (fs/btrfs/volumes.c:5088)
  Call Trace:
   <TASK>
   btrfs_ioctl (fs/btrfs/ioctl.c:3523 fs/btrfs/ioctl.c:5650)
   __x64_sys_ioctl
   do_syscall_64
   entry_SYSCALL_64_after_hwframe
   </TASK>

[CAUSE]
btrfs_balance() expects insert_balance_item() to return -EEXIST only
when a balance item is on disk, and a balance item to be on disk only
when fs_info->balance_ctl is set (a paused balance, or one recovered
at mount). So for a new balance it uses BUG_ON(ret == -EEXIST).

Both assumptions can be wrong:

- insert_balance_item() inserts the item, then commits the transaction.
  btrfs_commit_transaction() can fail without aborting the
  transaction, for example when btrfs_start_dirty_block_groups() fails
  to allocate a path. Then balance fails with -ENOMEM and
  balance_ctl is freed, but the item stays in the running transaction.
  The next balance ioctl finds the item and gets -EEXIST.

- When the transaction was aborted with -EEXIST, for example after
  "failed to run delayed ref ... -17" on a corrupted extent tree,
  btrfs_commit_transaction() returns the abort error, -EEXIST, and
  there is no balance item at all.

[FIX]
Mount turns any balance item on disk into a paused balance, so a new
balance can only find an item left by a balance of the same mount that
failed after inserting it.  No balance owns that item, so let a new
balance overwrite it, run, and delete it at the end as usual, as Filipe
suggested in [1].

For a resumed balance -EEXIST is the normal result.  Any other error,
including the -EEXIST of an aborted transaction for a new balance, now
goes to the existing error path.

The BUG_ON() checks on the return value go away, and the one on
fs_info->balance_ctl becomes an ASSERT().

Reported-by: syzbot+dc9291cf132691aeb478@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=dc9291cf132691aeb478
Link: https://lore.kernel.org/linux-btrfs/ZNzE6CFOzu9kDG+G@debian0.Home/ [1]
Fixes: 1bbc621ef284 ("Btrfs: allow block group cache writeout outside critical section in commit")
Cc: stable@vger.kernel.org # 4.1+
Suggested-by: Filipe Manana <fdmanana@kernel.org>
Signed-off-by: Siddharth Mishra <smish149@asu.edu>
---
 fs/btrfs/volumes.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3d4ef6725..4a8e5078b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3998,6 +3998,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
 
 	ret = btrfs_insert_empty_item(trans, root, path, &key,
 				      sizeof(*item));
+	if (ret == -EEXIST && !(bctl->flags & BTRFS_BALANCE_RESUME))
+		ret = 0;
 	if (ret)
 		goto out;
 
@@ -5081,17 +5083,17 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 	}
 
 	ret = insert_balance_item(fs_info, bctl);
-	if (ret && ret != -EEXIST)
+	if (ret == -EEXIST && (bctl->flags & BTRFS_BALANCE_RESUME))
+		ret = 0;
+	if (ret)
 		goto out;
 
 	if (!(bctl->flags & BTRFS_BALANCE_RESUME)) {
-		BUG_ON(ret == -EEXIST);
-		BUG_ON(fs_info->balance_ctl);
+		ASSERT(!fs_info->balance_ctl);
 		spin_lock(&fs_info->balance_lock);
 		fs_info->balance_ctl = bctl;
 		spin_unlock(&fs_info->balance_lock);
 	} else {
-		BUG_ON(ret != -EEXIST);
 		spin_lock(&fs_info->balance_lock);
 		update_balance_args(bctl);
 		spin_unlock(&fs_info->balance_lock);
-- 
2.54.0