[PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion

[PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion

[Qu Wenruo]

[CHANGELOG]
v2:
- Update the first patch to delaye memory allocation after all other
  checks


There are two bugs in the existing code base of btrfs-progs:

- Memory leak due to wrong error handling in btrfs_start_transaction()

- Empty rw device list due to missing error handling in create_chunk()

Just fix them.

Qu Wenruo (2):
  btrfs-progs: fix a memory leak when starting a transaction on fs with
    error
  btrfs-progs: fix an error path which can lead to empty device list

 kernel-shared/transaction.c | 13 +++++++------
 kernel-shared/volumes.c     | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 6 deletions(-)

-- 
2.35.1

[PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error

[Qu Wenruo]

Function btrfs_start_transaction() will allocate the memory
unconditionally, but if the fs has an aborted transaction we don't free
the allocated memory but return error directly.

Fix it by only allocate the new memory after all the checks.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/transaction.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c
index 0201226678ba..56828ee1714b 100644
--- a/kernel-shared/transaction.c
+++ b/kernel-shared/transaction.c
@@ -25,23 +25,24 @@ struct btrfs_trans_handle* btrfs_start_transaction(struct btrfs_root *root,
 		int num_blocks)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_trans_handle *h = kzalloc(sizeof(*h), GFP_NOFS);
-
+	struct btrfs_trans_handle *h;
+	
 	if (fs_info->transaction_aborted)
 		return ERR_PTR(-EROFS);
 
-	if (!h)
-		return ERR_PTR(-ENOMEM);
 	if (root->commit_root) {
 		error("commit_root already set when starting transaction");
-		kfree(h);
 		return ERR_PTR(-EINVAL);
 	}
 	if (fs_info->running_transaction) {
 		error("attempt to start transaction over already running one");
-		kfree(h);
 		return ERR_PTR(-EINVAL);
 	}
+
+	h = kzalloc(sizeof(*h), GFP_NOFS);
+	if (!h)
+		return ERR_PTR(-ENOMEM);
+
 	h->fs_info = fs_info;
 	fs_info->running_transaction = h;
 	fs_info->generation++;
-- 
2.35.1

[PATCH v2 2/2] btrfs-progs: fix an error path which can lead to empty device list

[Qu Wenruo]

[BUG]
With the incoming delayed chunk item insertion feature, there is a super
weird failure at mkfs/022:

  ====== RUN CHECK ./mkfs.btrfs -f --rootdir tmp.KnKpP5 -d dup -b 350M tests/test.img
  ...
  Checksum:           crc32c
  Number of devices:  0
  Devices:
     ID        SIZE  PATH

Note the "Number of devices: 0" line, this means our
fs_info->fs_devices->devices list is empty.

And since our rw device list is empty, we won't finish the mkfs with
proper superblock magic, and cause later btrfs check to fail.

[CAUSE]
Although the failure is only triggered by the incoming delayed chunk
item insertion feature, the bug itself is here for a while.

In btrfs_alloc_chunk(), we move rw devices to our @private_devs list
first, then in create_chunk(), we move it back to our rw devices list.

This dance is pretty dangerous, epsecially if btrfs_alloc_dev_extent()
failed inside create_chunk(), and current profile have multiple stripes
(including DUP), we will exit create_chunk() directly, without moving the
remaining devices in @private_devs list back to @dev_list.

Furthermore, btrfs_alloc_chunk() is expected to return -ENOSPC, as we
call btrfs_alloc_chunk() to pre-allocate chunks, and ignore the -ENOSPC
error if it's just a pre-allocation failure.

This existing error path can lead to the empty rw list seen above.

[FIX]
After create_chunk(), unconditionally move all devices in @private_devs
back to rw device list.

And add extra check to make sure our rw device list is never empty after
a chunk allocation attempt.

Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 kernel-shared/volumes.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index c7456394285a..97c09a1a4931 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -1559,6 +1559,21 @@ again:
 	}
 
 	ret = create_chunk(trans, info, &ctl, &private_devs);
+
+	/*
+	 * This can happen if above create_chunk() failed, we need to move all
+	 * devices back to dev_list.
+	 */
+	while (!list_empty(&private_devs)) {
+		device = list_entry(private_devs.next, struct btrfs_device,
+				    dev_list);
+		list_move(&device->dev_list, dev_list);
+	}
+	/*
+	 * All private devs moved back to @dev_list, now dev_list should not be
+	 * empty.
+	 */
+	ASSERT(!list_empty(dev_list));
 	*start = ctl.start;
 	*num_bytes = ctl.num_bytes;
 
-- 
2.35.1

Re: [PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 2/2] btrfs-progs: fix an error path which can lead to empty device list

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion

[David Sterba]

On Tue, Apr 19, 2022 at 07:17:40PM +0800, Qu Wenruo wrote:
> [CHANGELOG]
> v2:
> - Update the first patch to delaye memory allocation after all other
>   checks
> 
> 
> There are two bugs in the existing code base of btrfs-progs:
> 
> - Memory leak due to wrong error handling in btrfs_start_transaction()
> 
> - Empty rw device list due to missing error handling in create_chunk()
> 
> Just fix them.
> 
> Qu Wenruo (2):
>   btrfs-progs: fix a memory leak when starting a transaction on fs with
>     error
>   btrfs-progs: fix an error path which can lead to empty device list

Added to devel, thanks.