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

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

[Qu Wenruo]

I'm recently working on a prototype on seed device sprout at mkfs time,
and one core feature for it is delayed chunk item insertion.

Currently the delayed chunk item insertion is done, and mkfs sprout
prototype is pretty close to finish.

But during the development, I exposed some bugs in error handling path,
especially the 2nd one is exposed during testing, and it's there for a
while, affecting existing code base.

Currently my delayed chunk item insertion can survive all mkfs and fsck
tests, so I don't expect more bugs exposed, thus it's time for the pure
bug fixes first.

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 |  6 ++++--
 kernel-shared/volumes.c     | 15 +++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 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 the transaction_aborted
check.

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

diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c
index 0201226678ba..799520a0ef71 100644
--- a/kernel-shared/transaction.c
+++ b/kernel-shared/transaction.c
@@ -25,13 +25,15 @@ 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);
 
+	h = kzalloc(sizeof(*h), GFP_NOFS);
 	if (!h)
 		return ERR_PTR(-ENOMEM);
+
 	if (root->commit_root) {
 		error("commit_root already set when starting transaction");
 		kfree(h);
-- 
2.35.1

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

[Josef Bacik]

On Mon, Apr 18, 2022 at 09:10:07PM +0800, Qu Wenruo wrote:
> 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 the transaction_aborted
> check.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

[Andrei Borzenkov]

On 18.04.2022 16:10, Qu Wenruo wrote:
> 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 the transaction_aborted
> check.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  kernel-shared/transaction.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c
> index 0201226678ba..799520a0ef71 100644
> --- a/kernel-shared/transaction.c
> +++ b/kernel-shared/transaction.c
> @@ -25,13 +25,15 @@ 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);
>  
> +	h = kzalloc(sizeof(*h), GFP_NOFS);
>  	if (!h)
>  		return ERR_PTR(-ENOMEM);
> +
>  	if (root->commit_root) {
>  		error("commit_root already set when starting transaction");
>  		kfree(h);

If you are moving allocation of h anyway, why not move it beyond all
checks and delete redundant kfree(h)?

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

[Qu Wenruo]

On 2022/4/19 13:09, Andrei Borzenkov wrote:
> On 18.04.2022 16:10, Qu Wenruo wrote:
>> 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 the transaction_aborted
>> check.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   kernel-shared/transaction.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c
>> index 0201226678ba..799520a0ef71 100644
>> --- a/kernel-shared/transaction.c
>> +++ b/kernel-shared/transaction.c
>> @@ -25,13 +25,15 @@ 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);
>>
>> +	h = kzalloc(sizeof(*h), GFP_NOFS);
>>   	if (!h)
>>   		return ERR_PTR(-ENOMEM);
>> +
>>   	if (root->commit_root) {
>>   		error("commit_root already set when starting transaction");
>>   		kfree(h);
>
> If you are moving allocation of h anyway, why not move it beyond all
> checks and delete redundant kfree(h)?

Good idea.

Thanks,
Qu

[PATCH 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>
---
 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 2/2] btrfs-progs: fix an error path which can lead to empty device list

[Josef Bacik]

On Mon, Apr 18, 2022 at 09:10:08PM +0800, Qu Wenruo wrote:
> [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>

Thanks,

Josef