Skip to contentSkip to conversation

btrfs: Handle owner mismatch gracefully when walking up tree

PATCH Superseded Qu Wenruo 4 participants 7 messages Aug 1, 2018 – Aug 20, 2018 Btrfs file system

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

PATCH 1/1 btrfs: Handle owner mismatch gracefully when walking up tree

[BUG]
When mounting certain crafted image, btrfs will trigger kernel BUG_ON() when try to recover balance:
------

------------[ cut here ]------------
kernel BUG at fs/btrfs/extent-tree.c:8956!
invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 1 PID: 662 Comm: mount Not tainted 4.18.0-rc1-custom+ #10
RIP: 0010:walk_up_proc+0x336/0x480 [btrfs]
RSP: 0018:ffffb53540c9b890 EFLAGS: 00010202
Call Trace:
 walk_up_tree+0x172/0x1f0 [btrfs]
 btrfs_drop_snapshot+0x3a4/0x830 [btrfs]
 merge_reloc_roots+0xe1/0x1d0 [btrfs]
 btrfs_recover_relocation+0x3ea/0x420 [btrfs]
 open_ctree+0x1af3/0x1dd0 [btrfs]
 btrfs_mount_root+0x66b/0x740 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 btrfs_mount+0x16d/0x890 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 do_mount+0x1fd/0xda0
 ksys_mount+0xba/0xd0
 __x64_sys_mount+0x21/0x30
 do_syscall_64+0x60/0x210
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
---[ end trace d4344e4deee03435 ]---

------

[CAUSE]
Another extent tree corruption.

In this particular case, tree reloc root's owner is DATA_RELOC_TREE (should be TREE_RELOC_TREE), thus its backref is corrupted and we failed the owner check in walk_up_tree().

[FIX]
It's pretty hard to take care of every extent tree corruption, but at least we can remove such BUG_ON() and exit more gracefully.

And since in this particular image, DATA_RELOC_TREE and TREE_RELOC_TREE shares the same root (which is obviously invalid), we needs to make __del_reloc_root() more robust to detect such invalid share to avoid possible NULL dereference as root->node can be NULL in this case.

Link https://bugzilla.kernel.org/show_bug.cgi?id=200411
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

---
As always, the patch is also pushed to my github repo, along with other fuzzed images related fixes:
https://github.com/adam900710/linux/tree/tree_checker_enhance
(BTW, is it correct to indicate a branch like above?)

---
 fs/btrfs/extent-tree.c | 27 +++++++++++++++++++--------
 fs/btrfs/relocation.c  |  2 +-
 2 files changed, 20 insertions(+), 9 deletions(-)
fs/btrfs/extent-tree.c +19 -8
index da615ebc072e..5f4ca61348b5 100644
@@ -8949,17 +8949,26 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	}
 
 	if (eb == root->node) {
-		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = eb->start;
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(eb));
+		} else if (root->root_key.objectid != btrfs_header_owner(eb)) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;
+		}
 	} else {
-		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = path->nodes[level + 1]->start;
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(path->nodes[level + 1]));
+		} else if (root->root_key.objectid !=
+			   btrfs_header_owner(path->nodes[level + 1])) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;
+		}
 	}
 
 	btrfs_free_tree_block(trans, root, eb, parent, wc->refs[level] == 1);

@@ -9020,6 +9029,8 @@ static noinline int walk_up_tree(struct btrfs_trans_handle *trans,
 			ret = walk_up_proc(trans, root, path, wc);
 			if (ret > 0)
 				return 0;
+			if (ret < 0)
+				return ret;
 
 			if (path->locks[level]) {
 				btrfs_tree_unlock_rw(path->nodes[level],
fs/btrfs/relocation.c +1 -1
index a2fc0bd83a40..c64051d33d05 100644
@@ -1321,7 +1321,7 @@ static void __del_reloc_root(struct btrfs_root *root)
 	struct mapping_node *node = NULL;
 	struct reloc_control *rc = fs_info->reloc_ctl;
 
-	if (rc) {
+	if (rc && root->node) {
 		spin_lock(&rc->reloc_root_tree.lock);
 		rb_node = tree_search(&rc->reloc_root_tree.rb_root,
 				      root->node->start);
Signature · 1 line
2.18.0
#1 lore raw
Quoted 69 lines · Qu Wenruo

[BUG]
When mounting certain crafted image, btrfs will trigger kernel BUG_ON() when try to recover balance:
------

------------[ cut here ]------------
kernel BUG at fs/btrfs/extent-tree.c:8956!
invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 1 PID: 662 Comm: mount Not tainted 4.18.0-rc1-custom+ #10
RIP: 0010:walk_up_proc+0x336/0x480 [btrfs]
RSP: 0018:ffffb53540c9b890 EFLAGS: 00010202
Call Trace:
 walk_up_tree+0x172/0x1f0 [btrfs]
 btrfs_drop_snapshot+0x3a4/0x830 [btrfs]
 merge_reloc_roots+0xe1/0x1d0 [btrfs]
 btrfs_recover_relocation+0x3ea/0x420 [btrfs]
 open_ctree+0x1af3/0x1dd0 [btrfs]
 btrfs_mount_root+0x66b/0x740 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 btrfs_mount+0x16d/0x890 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 do_mount+0x1fd/0xda0
 ksys_mount+0xba/0xd0
 __x64_sys_mount+0x21/0x30
 do_syscall_64+0x60/0x210
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
---[ end trace d4344e4deee03435 ]---

------

[CAUSE]
Another extent tree corruption.

In this particular case, tree reloc root's owner is DATA_RELOC_TREE (should be TREE_RELOC_TREE), thus its backref is corrupted and we failed the owner check in walk_up_tree().

[FIX]
It's pretty hard to take care of every extent tree corruption, but at least we can remove such BUG_ON() and exit more gracefully.

And since in this particular image, DATA_RELOC_TREE and TREE_RELOC_TREE shares the same root (which is obviously invalid), we needs to make __del_reloc_root() more robust to detect such invalid share to avoid possible NULL dereference as root->node can be NULL in this case.

Link https://bugzilla.kernel.org/show_bug.cgi?id=200411
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

---
As always, the patch is also pushed to my github repo, along with other fuzzed images related fixes:
https://github.com/adam900710/linux/tree/tree_checker_enhance
(BTW, is it correct to indicate a branch like above?)

---
 fs/btrfs/extent-tree.c | 27 +++++++++++++++++++--------
 fs/btrfs/relocation.c  |  2 +-
 2 files changed, 20 insertions(+), 9 deletions(-)
fs/btrfs/extent-tree.c +1 -1
index da615ebc072e..5f4ca61348b5 100644
@@ -8949,17 +8949,26 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	}
 
 	if (eb == root->node) {
-		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8959

Quoted 10 lines
fs/btrfs/extent-tree.c +6 -3
 			parent = eb->start;
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(eb));
+		} else if (root->root_key.objectid != btrfs_header_owner(eb)) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

EINVAL or ECLEANUP?

Quoted 5 lines
fs/btrfs/extent-tree.c +2 -1
+		}
 	} else {
-		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = path->nodes[level + 1]->start;

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8970

Quoted 10 lines
fs/btrfs/extent-tree.c +7 -3
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(path->nodes[level + 1]));
+		} else if (root->root_key.objectid !=
+			   btrfs_header_owner(path->nodes[level + 1])) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

ditto

Quoted 27 lines
fs/btrfs/extent-tree.c +3 -0
+		}
 	}
 
 	btrfs_free_tree_block(trans, root, eb, parent, wc->refs[level] == 1);

@@ -9020,6 +9029,8 @@ static noinline int walk_up_tree(struct btrfs_trans_handle *trans,
 			ret = walk_up_proc(trans, root, path, wc);
 			if (ret > 0)
 				return 0;
+			if (ret < 0)
+				return ret;
 
 			if (path->locks[level]) {
 				btrfs_tree_unlock_rw(path->nodes[level],
fs/btrfs/relocation.c +1 -1
index a2fc0bd83a40..c64051d33d05 100644
@@ -1321,7 +1321,7 @@ static void __del_reloc_root(struct btrfs_root *root)
 	struct mapping_node *node = NULL;
 	struct reloc_control *rc = fs_info->reloc_ctl;
 
-	if (rc) {
+	if (rc && root->node) {
 		spin_lock(&rc->reloc_root_tree.lock);
 		rb_node = tree_search(&rc->reloc_root_tree.rb_root,
 				      root->node->start);
#2 lore raw
Quoted 69 lines · Nikolay Borisov
Quoted 66 lines · Qu Wenruo

[BUG]
When mounting certain crafted image, btrfs will trigger kernel BUG_ON() when try to recover balance:
------

------------[ cut here ]------------
kernel BUG at fs/btrfs/extent-tree.c:8956!
invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 1 PID: 662 Comm: mount Not tainted 4.18.0-rc1-custom+ #10
RIP: 0010:walk_up_proc+0x336/0x480 [btrfs]
RSP: 0018:ffffb53540c9b890 EFLAGS: 00010202
Call Trace:
 walk_up_tree+0x172/0x1f0 [btrfs]
 btrfs_drop_snapshot+0x3a4/0x830 [btrfs]
 merge_reloc_roots+0xe1/0x1d0 [btrfs]
 btrfs_recover_relocation+0x3ea/0x420 [btrfs]
 open_ctree+0x1af3/0x1dd0 [btrfs]
 btrfs_mount_root+0x66b/0x740 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 btrfs_mount+0x16d/0x890 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 do_mount+0x1fd/0xda0
 ksys_mount+0xba/0xd0
 __x64_sys_mount+0x21/0x30
 do_syscall_64+0x60/0x210
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
---[ end trace d4344e4deee03435 ]---

------

[CAUSE]
Another extent tree corruption.

In this particular case, tree reloc root's owner is DATA_RELOC_TREE (should be TREE_RELOC_TREE), thus its backref is corrupted and we failed the owner check in walk_up_tree().

[FIX]
It's pretty hard to take care of every extent tree corruption, but at least we can remove such BUG_ON() and exit more gracefully.

And since in this particular image, DATA_RELOC_TREE and TREE_RELOC_TREE shares the same root (which is obviously invalid), we needs to make __del_reloc_root() more robust to detect such invalid share to avoid possible NULL dereference as root->node can be NULL in this case.

Link https://bugzilla.kernel.org/show_bug.cgi?id=200411
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

---
As always, the patch is also pushed to my github repo, along with other fuzzed images related fixes:
https://github.com/adam900710/linux/tree/tree_checker_enhance
(BTW, is it correct to indicate a branch like above?)

---
 fs/btrfs/extent-tree.c | 27 +++++++++++++++++++--------
 fs/btrfs/relocation.c  |  2 +-
 2 files changed, 20 insertions(+), 9 deletions(-)
fs/btrfs/extent-tree.c +0 -0
index da615ebc072e..5f4ca61348b5 100644
@@ -8949,17 +8949,26 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	}
Quoted 15 lines

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8959

Quoted 13 lines
fs/btrfs/extent-tree.c +7 -4
 	if (eb == root->node) {
-		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = eb->start;
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(eb));
+		} else if (root->root_key.objectid != btrfs_header_owner(eb)) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

EINVAL or ECLEANUP?

Yep, also my concern here.

I have no bias here, and both makes its sense here.

EUCLEAN means it's something unexpected, but normally it's used in static check, no sure if it suits for runtime check.

Although EINVAL looks more suitable for runtime error, it is not a perfect errno either, as it's not something invalid from user, but the fs has something unexpected.

I'm all ears on this errno issue.

Thanks,
Qu

Quoted 49 lines
Quoted 6 lines
fs/btrfs/extent-tree.c +2 -1
+		}
 	} else {
-		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = path->nodes[level + 1]->start;

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8970

Quoted 10 lines
fs/btrfs/extent-tree.c +7 -3
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(path->nodes[level + 1]));
+		} else if (root->root_key.objectid !=
+			   btrfs_header_owner(path->nodes[level + 1])) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

ditto

Quoted 19 lines
fs/btrfs/extent-tree.c +3 -0
+		}
 	}
 
 	btrfs_free_tree_block(trans, root, eb, parent, wc->refs[level] == 1);

@@ -9020,6 +9029,8 @@ static noinline int walk_up_tree(struct btrfs_trans_handle *trans,
 			ret = walk_up_proc(trans, root, path, wc);
 			if (ret > 0)
 				return 0;
+			if (ret < 0)
+				return ret;
 
 			if (path->locks[level]) {
 				btrfs_tree_unlock_rw(path->nodes[level],
fs/btrfs/relocation.c +0 -0
index a2fc0bd83a40..c64051d33d05 100644

fs/btrfs/relocation.c static void __del_reloc_root(struct btrfs_root *root) · line 1327

Quoted 8 lines
fs/btrfs/relocation.c +1 -1
@@ -1321,7 +1321,7 @@ static void __del_reloc_root(struct btrfs_root *root)
 	struct mapping_node *node = NULL;
 	struct reloc_control *rc = fs_info->reloc_ctl;
 
-	if (rc) {
+	if (rc && root->node) {
 		spin_lock(&rc->reloc_root_tree.lock);
 		rb_node = tree_search(&rc->reloc_root_tree.rb_root,
 				      root->node->start);

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 88 lines · Qu Wenruo
Quoted 69 lines · Nikolay Borisov
Quoted 66 lines · Qu Wenruo

[BUG]
When mounting certain crafted image, btrfs will trigger kernel BUG_ON() when try to recover balance:
------

------------[ cut here ]------------
kernel BUG at fs/btrfs/extent-tree.c:8956!
invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 1 PID: 662 Comm: mount Not tainted 4.18.0-rc1-custom+ #10
RIP: 0010:walk_up_proc+0x336/0x480 [btrfs]
RSP: 0018:ffffb53540c9b890 EFLAGS: 00010202
Call Trace:
 walk_up_tree+0x172/0x1f0 [btrfs]
 btrfs_drop_snapshot+0x3a4/0x830 [btrfs]
 merge_reloc_roots+0xe1/0x1d0 [btrfs]
 btrfs_recover_relocation+0x3ea/0x420 [btrfs]
 open_ctree+0x1af3/0x1dd0 [btrfs]
 btrfs_mount_root+0x66b/0x740 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 btrfs_mount+0x16d/0x890 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 do_mount+0x1fd/0xda0
 ksys_mount+0xba/0xd0
 __x64_sys_mount+0x21/0x30
 do_syscall_64+0x60/0x210
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
---[ end trace d4344e4deee03435 ]---

------

[CAUSE]
Another extent tree corruption.

In this particular case, tree reloc root's owner is DATA_RELOC_TREE (should be TREE_RELOC_TREE), thus its backref is corrupted and we failed the owner check in walk_up_tree().

[FIX]
It's pretty hard to take care of every extent tree corruption, but at least we can remove such BUG_ON() and exit more gracefully.

And since in this particular image, DATA_RELOC_TREE and TREE_RELOC_TREE shares the same root (which is obviously invalid), we needs to make __del_reloc_root() more robust to detect such invalid share to avoid possible NULL dereference as root->node can be NULL in this case.

Link https://bugzilla.kernel.org/show_bug.cgi?id=200411
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

---
As always, the patch is also pushed to my github repo, along with other fuzzed images related fixes:
https://github.com/adam900710/linux/tree/tree_checker_enhance
(BTW, is it correct to indicate a branch like above?)

---
 fs/btrfs/extent-tree.c | 27 +++++++++++++++++++--------
 fs/btrfs/relocation.c  |  2 +-
 2 files changed, 20 insertions(+), 9 deletions(-)
fs/btrfs/extent-tree.c +0 -0
index da615ebc072e..5f4ca61348b5 100644
@@ -8949,17 +8949,26 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	}
Quoted 15 lines

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8959

Quoted 13 lines
fs/btrfs/extent-tree.c +7 -4
 	if (eb == root->node) {
-		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = eb->start;
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(eb));
+		} else if (root->root_key.objectid != btrfs_header_owner(eb)) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

EINVAL or ECLEANUP?

Quoted 6 lines

Yep, also my concern here.

I have no bias here, and both makes its sense here.

EUCLEAN means it's something unexpected, but normally it's used in static check, no sure if it suits for runtime check.

My thinking goes if something is an on-disk error (and fuzzed images fall in that category) then we should return EUCLEAN. If the owner can be mismatched only as a result of erroneous data on-disk which is then read and subsequently this code triggers then it's something induced due to an on-disk error.

Quoted 64 lines

Although EINVAL looks more suitable for runtime error, it is not a perfect errno either, as it's not something invalid from user, but the fs has something unexpected.

I'm all ears on this errno issue.

Thanks,
Qu

Quoted 38 lines
Quoted 6 lines
fs/btrfs/extent-tree.c +2 -1
+		}
 	} else {
-		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = path->nodes[level + 1]->start;

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8970

Quoted 10 lines
fs/btrfs/extent-tree.c +7 -3
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(path->nodes[level + 1]));
+		} else if (root->root_key.objectid !=
+			   btrfs_header_owner(path->nodes[level + 1])) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

ditto

Quoted 21 lines
fs/btrfs/extent-tree.c +3 -0
+		}
 	}
 
 	btrfs_free_tree_block(trans, root, eb, parent, wc->refs[level] == 1);

@@ -9020,6 +9029,8 @@ static noinline int walk_up_tree(struct btrfs_trans_handle *trans,
 			ret = walk_up_proc(trans, root, path, wc);
 			if (ret > 0)
 				return 0;
+			if (ret < 0)
+				return ret;
 
 			if (path->locks[level]) {
 				btrfs_tree_unlock_rw(path->nodes[level],
fs/btrfs/relocation.c +0 -0
index a2fc0bd83a40..c64051d33d05 100644
@@ -1321,7 +1321,7 @@ static void __del_reloc_root(struct btrfs_root *root)
 	struct mapping_node *node = NULL;
 	struct reloc_control *rc = fs_info->reloc_ctl;
Quoted 11 lines

fs/btrfs/relocation.c static void __del_reloc_root(struct btrfs_root *root) · line 1327

Quoted 6 lines
fs/btrfs/relocation.c +1 -1
-	if (rc) {
+	if (rc && root->node) {
 		spin_lock(&rc->reloc_root_tree.lock);
 		rb_node = tree_search(&rc->reloc_root_tree.rb_root,
 				      root->node->start);

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 95 lines · Nikolay Borisov
Quoted 92 lines · Qu Wenruo
Quoted 69 lines · Nikolay Borisov
Quoted 66 lines · Qu Wenruo

[BUG]
When mounting certain crafted image, btrfs will trigger kernel BUG_ON() when try to recover balance:
------

------------[ cut here ]------------
kernel BUG at fs/btrfs/extent-tree.c:8956!
invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 1 PID: 662 Comm: mount Not tainted 4.18.0-rc1-custom+ #10
RIP: 0010:walk_up_proc+0x336/0x480 [btrfs]
RSP: 0018:ffffb53540c9b890 EFLAGS: 00010202
Call Trace:
 walk_up_tree+0x172/0x1f0 [btrfs]
 btrfs_drop_snapshot+0x3a4/0x830 [btrfs]
 merge_reloc_roots+0xe1/0x1d0 [btrfs]
 btrfs_recover_relocation+0x3ea/0x420 [btrfs]
 open_ctree+0x1af3/0x1dd0 [btrfs]
 btrfs_mount_root+0x66b/0x740 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 btrfs_mount+0x16d/0x890 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 do_mount+0x1fd/0xda0
 ksys_mount+0xba/0xd0
 __x64_sys_mount+0x21/0x30
 do_syscall_64+0x60/0x210
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
---[ end trace d4344e4deee03435 ]---

------

[CAUSE]
Another extent tree corruption.

In this particular case, tree reloc root's owner is DATA_RELOC_TREE (should be TREE_RELOC_TREE), thus its backref is corrupted and we failed the owner check in walk_up_tree().

[FIX]
It's pretty hard to take care of every extent tree corruption, but at least we can remove such BUG_ON() and exit more gracefully.

And since in this particular image, DATA_RELOC_TREE and TREE_RELOC_TREE shares the same root (which is obviously invalid), we needs to make __del_reloc_root() more robust to detect such invalid share to avoid possible NULL dereference as root->node can be NULL in this case.

Link https://bugzilla.kernel.org/show_bug.cgi?id=200411
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

---
As always, the patch is also pushed to my github repo, along with other fuzzed images related fixes:
https://github.com/adam900710/linux/tree/tree_checker_enhance
(BTW, is it correct to indicate a branch like above?)

---
 fs/btrfs/extent-tree.c | 27 +++++++++++++++++++--------
 fs/btrfs/relocation.c  |  2 +-
 2 files changed, 20 insertions(+), 9 deletions(-)
fs/btrfs/extent-tree.c +0 -0
index da615ebc072e..5f4ca61348b5 100644
@@ -8949,17 +8949,26 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	}
Quoted 15 lines

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8959

Quoted 13 lines
fs/btrfs/extent-tree.c +7 -4
 	if (eb == root->node) {
-		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = eb->start;
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(eb));
+		} else if (root->root_key.objectid != btrfs_header_owner(eb)) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

EINVAL or ECLEANUP?

Yep, also my concern here.

I have no bias here, and both makes its sense here.

Quoted 8 lines
Quoted 2 lines

EUCLEAN means it's something unexpected, but normally it's used in static check, no sure if it suits for runtime check.

My thinking goes if something is an on-disk error (and fuzzed images fall in that category) then we should return EUCLEAN. If the owner can be mismatched only as a result of erroneous data on-disk which is then read and subsequently this code triggers then it's something induced due to an on-disk error.

Makes sense.

Does it also mean later BUG_ON() convert would also use EUCLEAN as most BUG_ON() is either some real bug or corrupted/fuzzed images?

Thanks,
Qu

Quoted 65 lines
Quoted 65 lines

Although EINVAL looks more suitable for runtime error, it is not a perfect errno either, as it's not something invalid from user, but the fs has something unexpected.

I'm all ears on this errno issue.

Thanks,
Qu

Quoted 38 lines
Quoted 6 lines
fs/btrfs/extent-tree.c +2 -1
+		}
 	} else {
-		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = path->nodes[level + 1]->start;

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8970

Quoted 10 lines
fs/btrfs/extent-tree.c +7 -3
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(path->nodes[level + 1]));
+		} else if (root->root_key.objectid !=
+			   btrfs_header_owner(path->nodes[level + 1])) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

ditto

Quoted 21 lines
fs/btrfs/extent-tree.c +3 -0
+		}
 	}
 
 	btrfs_free_tree_block(trans, root, eb, parent, wc->refs[level] == 1);

@@ -9020,6 +9029,8 @@ static noinline int walk_up_tree(struct btrfs_trans_handle *trans,
 			ret = walk_up_proc(trans, root, path, wc);
 			if (ret > 0)
 				return 0;
+			if (ret < 0)
+				return ret;
 
 			if (path->locks[level]) {
 				btrfs_tree_unlock_rw(path->nodes[level],
fs/btrfs/relocation.c +0 -0
index a2fc0bd83a40..c64051d33d05 100644
@@ -1321,7 +1321,7 @@ static void __del_reloc_root(struct btrfs_root *root)
 	struct mapping_node *node = NULL;
 	struct reloc_control *rc = fs_info->reloc_ctl;
Quoted 11 lines

fs/btrfs/relocation.c static void __del_reloc_root(struct btrfs_root *root) · line 1327

Quoted 6 lines
fs/btrfs/relocation.c +1 -1
-	if (rc) {
+	if (rc && root->node) {
 		spin_lock(&rc->reloc_root_tree.lock);
 		rb_node = tree_search(&rc->reloc_root_tree.rb_root,
 				      root->node->start);

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
Quoted 107 lines · Qu Wenruo
Quoted 95 lines · Nikolay Borisov
Quoted 92 lines · Qu Wenruo
Quoted 69 lines · Nikolay Borisov
Quoted 66 lines · Qu Wenruo

[BUG]
When mounting certain crafted image, btrfs will trigger kernel BUG_ON() when try to recover balance:
------

------------[ cut here ]------------
kernel BUG at fs/btrfs/extent-tree.c:8956!
invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 1 PID: 662 Comm: mount Not tainted 4.18.0-rc1-custom+ #10
RIP: 0010:walk_up_proc+0x336/0x480 [btrfs]
RSP: 0018:ffffb53540c9b890 EFLAGS: 00010202
Call Trace:
 walk_up_tree+0x172/0x1f0 [btrfs]
 btrfs_drop_snapshot+0x3a4/0x830 [btrfs]
 merge_reloc_roots+0xe1/0x1d0 [btrfs]
 btrfs_recover_relocation+0x3ea/0x420 [btrfs]
 open_ctree+0x1af3/0x1dd0 [btrfs]
 btrfs_mount_root+0x66b/0x740 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 btrfs_mount+0x16d/0x890 [btrfs]
 mount_fs+0x3b/0x16a
 vfs_kern_mount.part.9+0x54/0x140
 do_mount+0x1fd/0xda0
 ksys_mount+0xba/0xd0
 __x64_sys_mount+0x21/0x30
 do_syscall_64+0x60/0x210
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
---[ end trace d4344e4deee03435 ]---

------

[CAUSE]
Another extent tree corruption.

In this particular case, tree reloc root's owner is DATA_RELOC_TREE (should be TREE_RELOC_TREE), thus its backref is corrupted and we failed the owner check in walk_up_tree().

[FIX]
It's pretty hard to take care of every extent tree corruption, but at least we can remove such BUG_ON() and exit more gracefully.

And since in this particular image, DATA_RELOC_TREE and TREE_RELOC_TREE shares the same root (which is obviously invalid), we needs to make __del_reloc_root() more robust to detect such invalid share to avoid possible NULL dereference as root->node can be NULL in this case.

Link https://bugzilla.kernel.org/show_bug.cgi?id=200411
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

---
As always, the patch is also pushed to my github repo, along with other fuzzed images related fixes:
https://github.com/adam900710/linux/tree/tree_checker_enhance
(BTW, is it correct to indicate a branch like above?)

---
 fs/btrfs/extent-tree.c | 27 +++++++++++++++++++--------
 fs/btrfs/relocation.c  |  2 +-
 2 files changed, 20 insertions(+), 9 deletions(-)
fs/btrfs/extent-tree.c +0 -0
index da615ebc072e..5f4ca61348b5 100644
@@ -8949,17 +8949,26 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	}
Quoted 15 lines

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8959

Quoted 13 lines
fs/btrfs/extent-tree.c +7 -4
 	if (eb == root->node) {
-		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = eb->start;
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(eb));
+		} else if (root->root_key.objectid != btrfs_header_owner(eb)) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

EINVAL or ECLEANUP?

Yep, also my concern here.

I have no bias here, and both makes its sense here.

Quoted 8 lines
Quoted 2 lines

EUCLEAN means it's something unexpected, but normally it's used in static check, no sure if it suits for runtime check.

My thinking goes if something is an on-disk error (and fuzzed images fall in that category) then we should return EUCLEAN. If the owner can be mismatched only as a result of erroneous data on-disk which is then read and subsequently this code triggers then it's something induced due to an on-disk error.

Quoted 4 lines

Makes sense.

Does it also mean later BUG_ON() convert would also use EUCLEAN as most BUG_ON() is either some real bug or corrupted/fuzzed images?

If you refer to the next hunk the patch then I'd say yes.

Quoted 70 lines

Thanks,
Qu

Quoted 65 lines
Quoted 65 lines

Although EINVAL looks more suitable for runtime error, it is not a perfect errno either, as it's not something invalid from user, but the fs has something unexpected.

I'm all ears on this errno issue.

Thanks,
Qu

Quoted 38 lines
Quoted 6 lines
fs/btrfs/extent-tree.c +2 -1
+		}
 	} else {
-		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = path->nodes[level + 1]->start;

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8970

Quoted 10 lines
fs/btrfs/extent-tree.c +7 -3
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(path->nodes[level + 1]));
+		} else if (root->root_key.objectid !=
+			   btrfs_header_owner(path->nodes[level + 1])) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;

ditto

Quoted 21 lines
fs/btrfs/extent-tree.c +3 -0
+		}
 	}
 
 	btrfs_free_tree_block(trans, root, eb, parent, wc->refs[level] == 1);

@@ -9020,6 +9029,8 @@ static noinline int walk_up_tree(struct btrfs_trans_handle *trans,
 			ret = walk_up_proc(trans, root, path, wc);
 			if (ret > 0)
 				return 0;
+			if (ret < 0)
+				return ret;
 
 			if (path->locks[level]) {
 				btrfs_tree_unlock_rw(path->nodes[level],
fs/btrfs/relocation.c +0 -0
index a2fc0bd83a40..c64051d33d05 100644
@@ -1321,7 +1321,7 @@ static void __del_reloc_root(struct btrfs_root *root)
 	struct mapping_node *node = NULL;
 	struct reloc_control *rc = fs_info->reloc_ctl;
Quoted 11 lines

fs/btrfs/relocation.c static void __del_reloc_root(struct btrfs_root *root) · line 1327

Quoted 6 lines
fs/btrfs/relocation.c +1 -1
-	if (rc) {
+	if (rc && root->node) {
 		spin_lock(&rc->reloc_root_tree.lock);
 		rb_node = tree_search(&rc->reloc_root_tree.rb_root,
 				      root->node->start);

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw
In reply to Qu Wenruo
Quoted 22 lines · Qu Wenruo
fs/btrfs/extent-tree.c +9 -6
@@ -8949,17 +8949,26 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	}
 
 	if (eb == root->node) {
-		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = eb->start;
-		else
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(eb));
+		} else if (root->root_key.objectid != btrfs_header_owner(eb)) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;
+		}
 	} else {
-		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+		if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			parent = path->nodes[level + 1]->start;
-		else

fs/btrfs/extent-tree.c static noinline int walk_up_proc(struct btrfs_trans_handle *trans, · line 8971

Quoted 10 lines
fs/btrfs/extent-tree.c +8 -2
-			BUG_ON(root->root_key.objectid !=
-			       btrfs_header_owner(path->nodes[level + 1]));
+		} else if (root->root_key.objectid !=
+			   btrfs_header_owner(path->nodes[level + 1])) {
+			btrfs_err_rl(fs_info,
+			"unexpected tree owner, have %llu expect %llu",
+				     btrfs_header_owner(eb),
+				     root->root_key.objectid);
+			return -EINVAL;
+		}

Same code in both blocks, please merge them and add a label instead.

The suitable error code is EUCLEAN, as mentioned in the therad.