* [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion @ 2022-04-19 11:17 Qu Wenruo 2022-04-19 11:17 ` [PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error Qu Wenruo ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Qu Wenruo @ 2022-04-19 11:17 UTC (permalink / raw) To: linux-btrfs [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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error 2022-04-19 11:17 [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion Qu Wenruo @ 2022-04-19 11:17 ` Qu Wenruo 2022-04-19 11:23 ` Johannes Thumshirn 2022-04-19 11:17 ` [PATCH v2 2/2] btrfs-progs: fix an error path which can lead to empty device list Qu Wenruo 2022-04-25 16:56 ` [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion David Sterba 2 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2022-04-19 11:17 UTC (permalink / raw) To: linux-btrfs 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error 2022-04-19 11:17 ` [PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error Qu Wenruo @ 2022-04-19 11:23 ` Johannes Thumshirn 0 siblings, 0 replies; 6+ messages in thread From: Johannes Thumshirn @ 2022-04-19 11:23 UTC (permalink / raw) To: Qu Wenruo, linux-btrfs Looks good, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] btrfs-progs: fix an error path which can lead to empty device list 2022-04-19 11:17 [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion Qu Wenruo 2022-04-19 11:17 ` [PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error Qu Wenruo @ 2022-04-19 11:17 ` Qu Wenruo 2022-04-19 11:24 ` Johannes Thumshirn 2022-04-25 16:56 ` [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion David Sterba 2 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2022-04-19 11:17 UTC (permalink / raw) To: linux-btrfs; +Cc: Josef Bacik [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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] btrfs-progs: fix an error path which can lead to empty device list 2022-04-19 11:17 ` [PATCH v2 2/2] btrfs-progs: fix an error path which can lead to empty device list Qu Wenruo @ 2022-04-19 11:24 ` Johannes Thumshirn 0 siblings, 0 replies; 6+ messages in thread From: Johannes Thumshirn @ 2022-04-19 11:24 UTC (permalink / raw) To: Qu Wenruo, linux-btrfs; +Cc: Josef Bacik Looks good, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion 2022-04-19 11:17 [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion Qu Wenruo 2022-04-19 11:17 ` [PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error Qu Wenruo 2022-04-19 11:17 ` [PATCH v2 2/2] btrfs-progs: fix an error path which can lead to empty device list Qu Wenruo @ 2022-04-25 16:56 ` David Sterba 2 siblings, 0 replies; 6+ messages in thread From: David Sterba @ 2022-04-25 16:56 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-25 17:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-19 11:17 [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion Qu Wenruo 2022-04-19 11:17 ` [PATCH v2 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error Qu Wenruo 2022-04-19 11:23 ` Johannes Thumshirn 2022-04-19 11:17 ` [PATCH v2 2/2] btrfs-progs: fix an error path which can lead to empty device list Qu Wenruo 2022-04-19 11:24 ` Johannes Thumshirn 2022-04-25 16:56 ` [PATCH v2 0/2] btrfs-progs: bug fixes exposed during delayed chunk items insertion David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.