* [PATCH 6.6] btrfs: do not BUG_ON() when freeing tree block after error
@ 2024-11-29 6:27 bin.lan.cn
2024-11-29 20:03 ` Sasha Levin
0 siblings, 1 reply; 2+ messages in thread
From: bin.lan.cn @ 2024-11-29 6:27 UTC (permalink / raw)
To: stable, fdmanana
From: Filipe Manana <fdmanana@suse.com>
[ Upstream commit bb3868033a4cccff7be57e9145f2117cbdc91c11 ]
When freeing a tree block, at btrfs_free_tree_block(), if we fail to
create a delayed reference we don't deal with the error and just do a
BUG_ON(). The error most likely to happen is -ENOMEM, and we have a
comment mentioning that only -ENOMEM can happen, but that is not true,
because in case qgroups are enabled any error returned from
btrfs_qgroup_trace_extent_post() (can be -EUCLEAN or anything returned
from btrfs_search_slot() for example) can be propagated back to
btrfs_free_tree_block().
So stop doing a BUG_ON() and return the error to the callers and make
them abort the transaction to prevent leaking space. Syzbot was
triggering this, likely due to memory allocation failure injection.
Reported-by: syzbot+a306f914b4d01b3958fe@syzkaller.appspotmail.com
Link: https://lore.kernel.org/linux-btrfs/000000000000fcba1e05e998263c@google.com/
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
[ Resolve minor conflicts ]
Signed-off-by: Bin Lan <bin.lan.cn@windriver.com>
---
fs/btrfs/ctree.c | 51 ++++++++++++++++++++++++++++++--------
fs/btrfs/extent-tree.c | 22 +++++++++-------
fs/btrfs/extent-tree.h | 8 +++---
fs/btrfs/free-space-tree.c | 10 +++++---
fs/btrfs/ioctl.c | 6 ++++-
fs/btrfs/qgroup.c | 6 +++--
6 files changed, 74 insertions(+), 29 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 2eb4e03080ac..bb5d317fcdbe 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -617,10 +617,16 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
atomic_inc(&cow->refs);
rcu_assign_pointer(root->node, cow);
- btrfs_free_tree_block(trans, btrfs_root_id(root), buf,
- parent_start, last_ref);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), buf,
+ parent_start, last_ref);
free_extent_buffer(buf);
add_root_to_dirty_list(root);
+ if (ret < 0) {
+ btrfs_tree_unlock(cow);
+ free_extent_buffer(cow);
+ btrfs_abort_transaction(trans, ret);
+ return ret;
+ }
} else {
WARN_ON(trans->transid != btrfs_header_generation(parent));
ret = btrfs_tree_mod_log_insert_key(parent, parent_slot,
@@ -645,8 +651,14 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
return ret;
}
}
- btrfs_free_tree_block(trans, btrfs_root_id(root), buf,
- parent_start, last_ref);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), buf,
+ parent_start, last_ref);
+ if (ret < 0) {
+ btrfs_tree_unlock(cow);
+ free_extent_buffer(cow);
+ btrfs_abort_transaction(trans, ret);
+ return ret;
+ }
}
if (unlock_orig)
btrfs_tree_unlock(buf);
@@ -1121,9 +1133,13 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
free_extent_buffer(mid);
root_sub_used(root, mid->len);
- btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
/* once for the root ptr */
free_extent_buffer_stale(mid);
+ if (ret < 0) {
+ btrfs_abort_transaction(trans, ret);
+ goto out;
+ }
return 0;
}
if (btrfs_header_nritems(mid) >
@@ -1191,10 +1207,14 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
goto out;
}
root_sub_used(root, right->len);
- btrfs_free_tree_block(trans, btrfs_root_id(root), right,
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), right,
0, 1);
free_extent_buffer_stale(right);
right = NULL;
+ if (ret < 0) {
+ btrfs_abort_transaction(trans, ret);
+ goto out;
+ }
} else {
struct btrfs_disk_key right_key;
btrfs_node_key(right, &right_key, 0);
@@ -1249,9 +1269,13 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
goto out;
}
root_sub_used(root, mid->len);
- btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
free_extent_buffer_stale(mid);
mid = NULL;
+ if (ret < 0) {
+ btrfs_abort_transaction(trans, ret);
+ goto out;
+ }
} else {
/* update the parent key to reflect our changes */
struct btrfs_disk_key mid_key;
@@ -3022,7 +3046,11 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
old = root->node;
ret = btrfs_tree_mod_log_insert_root(root->node, c, false);
if (ret < 0) {
- btrfs_free_tree_block(trans, btrfs_root_id(root), c, 0, 1);
+ int ret2;
+
+ ret2 = btrfs_free_tree_block(trans, btrfs_root_id(root), c, 0, 1);
+ if (ret2 < 0)
+ btrfs_abort_transaction(trans, ret2);
btrfs_tree_unlock(c);
free_extent_buffer(c);
return ret;
@@ -4587,9 +4615,12 @@ static noinline int btrfs_del_leaf(struct btrfs_trans_handle *trans,
root_sub_used(root, leaf->len);
atomic_inc(&leaf->refs);
- btrfs_free_tree_block(trans, btrfs_root_id(root), leaf, 0, 1);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), leaf, 0, 1);
free_extent_buffer_stale(leaf);
- return 0;
+ if (ret < 0)
+ btrfs_abort_transaction(trans, ret);
+
+ return ret;
}
/*
* delete the item at the leaf level in path. If that empties
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index b3680e1c7054..94fc86c9c65e 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3290,10 +3290,10 @@ static noinline int check_ref_cleanup(struct btrfs_trans_handle *trans,
return 0;
}
-void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
- u64 root_id,
- struct extent_buffer *buf,
- u64 parent, int last_ref)
+int btrfs_free_tree_block(struct btrfs_trans_handle *trans,
+ u64 root_id,
+ struct extent_buffer *buf,
+ u64 parent, int last_ref)
{
struct btrfs_fs_info *fs_info = trans->fs_info;
struct btrfs_ref generic_ref = { 0 };
@@ -3307,7 +3307,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
if (root_id != BTRFS_TREE_LOG_OBJECTID) {
btrfs_ref_tree_mod(fs_info, &generic_ref);
ret = btrfs_add_delayed_tree_ref(trans, &generic_ref, NULL);
- BUG_ON(ret); /* -ENOMEM */
+ if (ret < 0)
+ return ret;
}
if (last_ref && btrfs_header_generation(buf) == trans->transid) {
@@ -3371,6 +3372,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
*/
clear_bit(EXTENT_BUFFER_CORRUPT, &buf->bflags);
}
+ return 0;
}
/* Can return -ENOMEM */
@@ -5474,7 +5476,7 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
struct walk_control *wc)
{
struct btrfs_fs_info *fs_info = root->fs_info;
- int ret;
+ int ret = 0;
int level = wc->level;
struct extent_buffer *eb = path->nodes[level];
u64 parent = 0;
@@ -5565,12 +5567,14 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
goto owner_mismatch;
}
- btrfs_free_tree_block(trans, btrfs_root_id(root), eb, parent,
- wc->refs[level] == 1);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), eb, parent,
+ wc->refs[level] == 1);
+ if (ret < 0)
+ btrfs_abort_transaction(trans, ret);
out:
wc->refs[level] = 0;
wc->flags[level] = 0;
- return 0;
+ return ret;
owner_mismatch:
btrfs_err_rl(fs_info, "unexpected tree owner, have %llu expect %llu",
diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
index 88c249c37516..ef1c1c99294e 100644
--- a/fs/btrfs/extent-tree.h
+++ b/fs/btrfs/extent-tree.h
@@ -114,10 +114,10 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
int level, u64 hint,
u64 empty_size,
enum btrfs_lock_nesting nest);
-void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
- u64 root_id,
- struct extent_buffer *buf,
- u64 parent, int last_ref);
+int btrfs_free_tree_block(struct btrfs_trans_handle *trans,
+ u64 root_id,
+ struct extent_buffer *buf,
+ u64 parent, int last_ref);
int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
struct btrfs_root *root, u64 owner,
u64 offset, u64 ram_bytes,
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 7b598b070700..a0d8160b5375 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1289,10 +1289,14 @@ int btrfs_delete_free_space_tree(struct btrfs_fs_info *fs_info)
btrfs_tree_lock(free_space_root->node);
btrfs_clear_buffer_dirty(trans, free_space_root->node);
btrfs_tree_unlock(free_space_root->node);
- btrfs_free_tree_block(trans, btrfs_root_id(free_space_root),
- free_space_root->node, 0, 1);
-
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(free_space_root),
+ free_space_root->node, 0, 1);
btrfs_put_root(free_space_root);
+ if (ret < 0) {
+ btrfs_abort_transaction(trans, ret);
+ btrfs_end_transaction(trans);
+ return ret;
+ }
return btrfs_commit_transaction(trans);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 5f0c9c3f3bbf..ae6806bc3929 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -707,6 +707,8 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
ret = btrfs_insert_root(trans, fs_info->tree_root, &key,
root_item);
if (ret) {
+ int ret2;
+
/*
* Since we don't abort the transaction in this case, free the
* tree block so that we don't leak space and leave the
@@ -717,7 +719,9 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
btrfs_tree_lock(leaf);
btrfs_clear_buffer_dirty(trans, leaf);
btrfs_tree_unlock(leaf);
- btrfs_free_tree_block(trans, objectid, leaf, 0, 1);
+ ret2 = btrfs_free_tree_block(trans, objectid, leaf, 0, 1);
+ if (ret2 < 0)
+ btrfs_abort_transaction(trans, ret2);
free_extent_buffer(leaf);
goto out;
}
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 74b82390fe84..1b9f4f16d124 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1320,9 +1320,11 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
btrfs_tree_lock(quota_root->node);
btrfs_clear_buffer_dirty(trans, quota_root->node);
btrfs_tree_unlock(quota_root->node);
- btrfs_free_tree_block(trans, btrfs_root_id(quota_root),
- quota_root->node, 0, 1);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(quota_root),
+ quota_root->node, 0, 1);
+ if (ret < 0)
+ btrfs_abort_transaction(trans, ret);
out:
btrfs_put_root(quota_root);
--
2.34.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH 6.6] btrfs: do not BUG_ON() when freeing tree block after error
2024-11-29 6:27 [PATCH 6.6] btrfs: do not BUG_ON() when freeing tree block after error bin.lan.cn
@ 2024-11-29 20:03 ` Sasha Levin
0 siblings, 0 replies; 2+ messages in thread
From: Sasha Levin @ 2024-11-29 20:03 UTC (permalink / raw)
To: stable; +Cc: bin.lan.cn, Sasha Levin
[ Sasha's backport helper bot ]
Hi,
The upstream commit SHA1 provided is correct: bb3868033a4cccff7be57e9145f2117cbdc91c11
WARNING: Author mismatch between patch and upstream commit:
Backport author: bin.lan.cn@eng.windriver.com
Commit author: Filipe Manana <fdmanana@suse.com>
Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.11.y | Present (exact SHA1)
6.6.y | Not found
Note: The patch differs from the upstream commit:
---
1: bb3868033a4cc ! 1: 7b3a3b03bd7a2 btrfs: do not BUG_ON() when freeing tree block after error
@@ Metadata
## Commit message ##
btrfs: do not BUG_ON() when freeing tree block after error
+ [ Upstream commit bb3868033a4cccff7be57e9145f2117cbdc91c11 ]
+
When freeing a tree block, at btrfs_free_tree_block(), if we fail to
create a delayed reference we don't deal with the error and just do a
BUG_ON(). The error most likely to happen is -ENOMEM, and we have a
@@ Commit message
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
+ [ Resolve minor conflicts ]
+ Signed-off-by: Bin Lan <bin.lan.cn@windriver.com>
## fs/btrfs/ctree.c ##
-@@ fs/btrfs/ctree.c: int btrfs_force_cow_block(struct btrfs_trans_handle *trans,
+@@ fs/btrfs/ctree.c: static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
atomic_inc(&cow->refs);
rcu_assign_pointer(root->node, cow);
@@ fs/btrfs/ctree.c: int btrfs_force_cow_block(struct btrfs_trans_handle *trans,
} else {
WARN_ON(trans->transid != btrfs_header_generation(parent));
ret = btrfs_tree_mod_log_insert_key(parent, parent_slot,
-@@ fs/btrfs/ctree.c: int btrfs_force_cow_block(struct btrfs_trans_handle *trans,
+@@ fs/btrfs/ctree.c: static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
return ret;
}
}
@@ fs/btrfs/ctree.c: int btrfs_force_cow_block(struct btrfs_trans_handle *trans,
@@ fs/btrfs/ctree.c: static noinline int balance_level(struct btrfs_trans_handle *trans,
free_extent_buffer(mid);
- root_sub_used_bytes(root);
+ root_sub_used(root, mid->len);
- btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
/* once for the root ptr */
@@ fs/btrfs/ctree.c: static noinline int balance_level(struct btrfs_trans_handle *t
@@ fs/btrfs/ctree.c: static noinline int balance_level(struct btrfs_trans_handle *trans,
goto out;
}
- root_sub_used_bytes(root);
+ root_sub_used(root, right->len);
- btrfs_free_tree_block(trans, btrfs_root_id(root), right,
-- 0, 1);
-+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root),
-+ right, 0, 1);
++ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), right,
+ 0, 1);
free_extent_buffer_stale(right);
right = NULL;
+ if (ret < 0) {
@@ fs/btrfs/ctree.c: static noinline int balance_level(struct btrfs_trans_handle *t
@@ fs/btrfs/ctree.c: static noinline int balance_level(struct btrfs_trans_handle *trans,
goto out;
}
- root_sub_used_bytes(root);
+ root_sub_used(root, mid->len);
- btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
+ ret = btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
free_extent_buffer_stale(mid);
@@ fs/btrfs/ctree.c: static noinline int insert_new_root(struct btrfs_trans_handle
free_extent_buffer(c);
return ret;
@@ fs/btrfs/ctree.c: static noinline int btrfs_del_leaf(struct btrfs_trans_handle *trans,
- root_sub_used_bytes(root);
+ root_sub_used(root, leaf->len);
atomic_inc(&leaf->refs);
- btrfs_free_tree_block(trans, btrfs_root_id(root), leaf, 0, 1);
@@ fs/btrfs/extent-tree.c: static noinline int check_ref_cleanup(struct btrfs_trans
+ u64 parent, int last_ref)
{
struct btrfs_fs_info *fs_info = trans->fs_info;
- struct btrfs_block_group *bg;
+ struct btrfs_ref generic_ref = { 0 };
@@ fs/btrfs/extent-tree.c: void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
- btrfs_init_tree_ref(&generic_ref, btrfs_header_level(buf), 0, false);
+ if (root_id != BTRFS_TREE_LOG_OBJECTID) {
btrfs_ref_tree_mod(fs_info, &generic_ref);
ret = btrfs_add_delayed_tree_ref(trans, &generic_ref, NULL);
- BUG_ON(ret); /* -ENOMEM */
@@ fs/btrfs/extent-tree.c: void btrfs_free_tree_block(struct btrfs_trans_handle *tr
+ return ret;
}
- if (!last_ref)
-- return;
-+ return 0;
-
- if (btrfs_header_generation(buf) != trans->transid)
- goto out;
+ if (last_ref && btrfs_header_generation(buf) == trans->transid) {
@@ fs/btrfs/extent-tree.c: void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
- * matter anymore.
- */
- clear_bit(EXTENT_BUFFER_CORRUPT, &buf->bflags);
+ */
+ clear_bit(EXTENT_BUFFER_CORRUPT, &buf->bflags);
+ }
+ return 0;
}
@@ fs/btrfs/extent-tree.c: static noinline int walk_up_proc(struct btrfs_trans_hand
## fs/btrfs/extent-tree.h ##
@@ fs/btrfs/extent-tree.h: struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
+ int level, u64 hint,
u64 empty_size,
- u64 reloc_src_root,
enum btrfs_lock_nesting nest);
-void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
- u64 root_id,
@@ fs/btrfs/free-space-tree.c: int btrfs_delete_free_space_tree(struct btrfs_fs_inf
+ }
return btrfs_commit_transaction(trans);
- }
+
## fs/btrfs/ioctl.c ##
@@ fs/btrfs/ioctl.c: static noinline int create_subvol(struct mnt_idmap *idmap,
---
Results of testing on various branches:
| Branch | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.6.y | Success | Success |
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-11-29 20:03 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-29 6:27 [PATCH 6.6] btrfs: do not BUG_ON() when freeing tree block after error bin.lan.cn
2024-11-29 20:03 ` Sasha Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox