diff mbox series

[3/3] btrfs-progs: Handle error properly in btrfs_commit_transaction()

Message ID 20190416071526.3576-4-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs-progs: Handle error properly in btrfs_commit_transaction() | expand

Commit Message

Qu Wenruo April 16, 2019, 7:15 a.m. UTC 
[BUG]
When running fuzz-tests/003 and fuzz-tests/009, btrfs-progs will crash
due to BUG_ON().

[CAUSE]
We abused BUG_ON() in btrfs_commit_transaction(), which is one of the
most error prone function for fuzzed images.

Currently to cleanup the aborted transaction, we only need to clean up
the only per-transaction data: delayed refs.

This patch will introduce a new function, btrfs_destroy_delayed_refs()
to cleanup delayed refs when we failed to commit transaction.

With that function, we will gently destroy per-trans delayed ref, and
remove the BUG_ON()s in btrfs_commit_transaction().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 delayed-ref.c | 24 ++++++++++++++++++++++++
 delayed-ref.h |  5 +++++
 extent-tree.c |  6 +++---
 transaction.c | 20 ++++++++++++++------
 4 files changed, 46 insertions(+), 9 deletions(-)

Comments

Nikolay Borisov April 16, 2019, 7:48 a.m. UTC | #1 
On 16.04.19 г. 10:15 ч., Qu Wenruo wrote:
> [BUG]
> When running fuzz-tests/003 and fuzz-tests/009, btrfs-progs will crash
> due to BUG_ON().
> 
> [CAUSE]
> We abused BUG_ON() in btrfs_commit_transaction(), which is one of the
> most error prone function for fuzzed images.
> 
> Currently to cleanup the aborted transaction, we only need to clean up
> the only per-transaction data: delayed refs.
> 
> This patch will introduce a new function, btrfs_destroy_delayed_refs()
> to cleanup delayed refs when we failed to commit transaction.
> 
> With that function, we will gently destroy per-trans delayed ref, and
> remove the BUG_ON()s in btrfs_commit_transaction().
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Though see below for a suggestion of further cleanup

> ---
>  delayed-ref.c | 24 ++++++++++++++++++++++++
>  delayed-ref.h |  5 +++++
>  extent-tree.c |  6 +++---
>  transaction.c | 20 ++++++++++++++------
>  4 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/delayed-ref.c b/delayed-ref.c
> index 9974dbbd4c6b..69f8be34fe18 100644
> --- a/delayed-ref.c
> +++ b/delayed-ref.c
> @@ -605,3 +605,27 @@ free_ref:
>  
>  	return -ENOMEM;
>  }
> +
> +void btrfs_destroy_delayed_refs(struct btrfs_trans_handle *trans)
> +{
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	struct rb_node *node;
> +	struct btrfs_delayed_ref_root *delayed_refs;
> +
> +	delayed_refs = &trans->delayed_refs;
> +	if (RB_EMPTY_ROOT(&delayed_refs->href_root))
> +		return;
> +	while ((node = rb_first(&delayed_refs->href_root)) != NULL) {
> +		struct btrfs_delayed_ref_head *head;
> +		struct btrfs_delayed_ref_node *ref;
> +		struct rb_node *n;
> +
> +		head = rb_entry(node, struct btrfs_delayed_ref_head, href_node);
> +		while ((n = rb_first(&head->ref_tree)) != NULL) {
> +			ref = rb_entry(n, struct btrfs_delayed_ref_node,
> +				       ref_node);
> +			drop_delayed_ref(trans, delayed_refs, head, ref);
> +		}
> +		ASSERT(cleanup_ref_head(trans, fs_info, head) == 0);
> +	}
> +}
> diff --git a/delayed-ref.h b/delayed-ref.h
> index 30a68b2a278c..4ff3eaa929ba 100644
> --- a/delayed-ref.h
> +++ b/delayed-ref.h
> @@ -205,4 +205,9 @@ btrfs_delayed_node_to_tree_ref(struct btrfs_delayed_ref_node *node)
>  {
>  	return container_of(node, struct btrfs_delayed_tree_ref, node);
>  }
> +
> +int cleanup_ref_head(struct btrfs_trans_handle *trans,
> +		     struct btrfs_fs_info *fs_info,
> +		     struct btrfs_delayed_ref_head *head);
> +void btrfs_destroy_delayed_refs(struct btrfs_trans_handle *trans);
>  #endif
> diff --git a/extent-tree.c b/extent-tree.c
> index 1140ebfc61ff..e62ee8c2ba13 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -4085,9 +4085,9 @@ static void unselect_delayed_ref_head(struct btrfs_delayed_ref_root *delayed_ref
>  	delayed_refs->num_heads_ready++;
>  }
>  
> -static int cleanup_ref_head(struct btrfs_trans_handle *trans,
> -			    struct btrfs_fs_info *fs_info,
> -			    struct btrfs_delayed_ref_head *head)
> +int cleanup_ref_head(struct btrfs_trans_handle *trans,
> +		     struct btrfs_fs_info *fs_info,
> +		     struct btrfs_delayed_ref_head *head)

You can create a followup patch that removes the fs_info argument from
cleanup_ref_head to make it identical to the kernel version.

>  {
>  	struct btrfs_delayed_ref_root *delayed_refs;
>  
> diff --git a/transaction.c b/transaction.c
> index 8d15eb11c7b5..138e10f0d6cc 100644
> --- a/transaction.c
> +++ b/transaction.c
> @@ -17,6 +17,7 @@
>  #include "kerncompat.h"
>  #include "disk-io.h"
>  #include "transaction.h"
> +#include "delayed-ref.h"
>  
>  #include "messages.h"
>  
> @@ -165,7 +166,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>  	 * consistent
>  	 */
>  	ret = btrfs_run_delayed_refs(trans, -1);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto error;
>  
>  	if (root->commit_root == root->node)
>  		goto commit_tree;
> @@ -182,21 +184,24 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>  	root->root_item.level = btrfs_header_level(root->node);
>  	ret = btrfs_update_root(trans, root->fs_info->tree_root,
>  				&root->root_key, &root->root_item);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto error;
>  
>  commit_tree:
>  	ret = commit_tree_roots(trans, fs_info);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto error;
>  	/*
>  	 * Ensure that all committed roots are properly accounted in the
>  	 * extent tree
>  	 */
>  	ret = btrfs_run_delayed_refs(trans, -1);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto error;
>  	btrfs_write_dirty_block_groups(trans);
>  	__commit_transaction(trans, root);
>  	if (ret < 0)
> -		goto out;
> +		goto error;
>  	ret = write_ctree_super(trans);
>  	btrfs_finish_extent_commit(trans);
>  	kfree(trans);
> @@ -204,7 +209,10 @@ commit_tree:
>  	root->commit_root = NULL;
>  	fs_info->running_transaction = NULL;
>  	fs_info->last_trans_committed = transid;
> -out:
> +	return ret;
> +error:
> +	btrfs_destroy_delayed_refs(trans);
> +	free(trans);
>  	return ret;
>  }
>  
>
diff mbox series

Patch

diff --git a/delayed-ref.c b/delayed-ref.c
index 9974dbbd4c6b..69f8be34fe18 100644
--- a/delayed-ref.c
+++ b/delayed-ref.c
@@ -605,3 +605,27 @@  free_ref:
 
 	return -ENOMEM;
 }
+
+void btrfs_destroy_delayed_refs(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct rb_node *node;
+	struct btrfs_delayed_ref_root *delayed_refs;
+
+	delayed_refs = &trans->delayed_refs;
+	if (RB_EMPTY_ROOT(&delayed_refs->href_root))
+		return;
+	while ((node = rb_first(&delayed_refs->href_root)) != NULL) {
+		struct btrfs_delayed_ref_head *head;
+		struct btrfs_delayed_ref_node *ref;
+		struct rb_node *n;
+
+		head = rb_entry(node, struct btrfs_delayed_ref_head, href_node);
+		while ((n = rb_first(&head->ref_tree)) != NULL) {
+			ref = rb_entry(n, struct btrfs_delayed_ref_node,
+				       ref_node);
+			drop_delayed_ref(trans, delayed_refs, head, ref);
+		}
+		ASSERT(cleanup_ref_head(trans, fs_info, head) == 0);
+	}
+}
diff --git a/delayed-ref.h b/delayed-ref.h
index 30a68b2a278c..4ff3eaa929ba 100644
--- a/delayed-ref.h
+++ b/delayed-ref.h
@@ -205,4 +205,9 @@  btrfs_delayed_node_to_tree_ref(struct btrfs_delayed_ref_node *node)
 {
 	return container_of(node, struct btrfs_delayed_tree_ref, node);
 }
+
+int cleanup_ref_head(struct btrfs_trans_handle *trans,
+		     struct btrfs_fs_info *fs_info,
+		     struct btrfs_delayed_ref_head *head);
+void btrfs_destroy_delayed_refs(struct btrfs_trans_handle *trans);
 #endif
diff --git a/extent-tree.c b/extent-tree.c
index 1140ebfc61ff..e62ee8c2ba13 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -4085,9 +4085,9 @@  static void unselect_delayed_ref_head(struct btrfs_delayed_ref_root *delayed_ref
 	delayed_refs->num_heads_ready++;
 }
 
-static int cleanup_ref_head(struct btrfs_trans_handle *trans,
-			    struct btrfs_fs_info *fs_info,
-			    struct btrfs_delayed_ref_head *head)
+int cleanup_ref_head(struct btrfs_trans_handle *trans,
+		     struct btrfs_fs_info *fs_info,
+		     struct btrfs_delayed_ref_head *head)
 {
 	struct btrfs_delayed_ref_root *delayed_refs;
 
diff --git a/transaction.c b/transaction.c
index 8d15eb11c7b5..138e10f0d6cc 100644
--- a/transaction.c
+++ b/transaction.c
@@ -17,6 +17,7 @@ 
 #include "kerncompat.h"
 #include "disk-io.h"
 #include "transaction.h"
+#include "delayed-ref.h"
 
 #include "messages.h"
 
@@ -165,7 +166,8 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	 * consistent
 	 */
 	ret = btrfs_run_delayed_refs(trans, -1);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto error;
 
 	if (root->commit_root == root->node)
 		goto commit_tree;
@@ -182,21 +184,24 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	root->root_item.level = btrfs_header_level(root->node);
 	ret = btrfs_update_root(trans, root->fs_info->tree_root,
 				&root->root_key, &root->root_item);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto error;
 
 commit_tree:
 	ret = commit_tree_roots(trans, fs_info);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto error;
 	/*
 	 * Ensure that all committed roots are properly accounted in the
 	 * extent tree
 	 */
 	ret = btrfs_run_delayed_refs(trans, -1);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto error;
 	btrfs_write_dirty_block_groups(trans);
 	__commit_transaction(trans, root);
 	if (ret < 0)
-		goto out;
+		goto error;
 	ret = write_ctree_super(trans);
 	btrfs_finish_extent_commit(trans);
 	kfree(trans);
@@ -204,7 +209,10 @@  commit_tree:
 	root->commit_root = NULL;
 	fs_info->running_transaction = NULL;
 	fs_info->last_trans_committed = transid;
-out:
+	return ret;
+error:
+	btrfs_destroy_delayed_refs(trans);
+	free(trans);
 	return ret;
 }