[PATCH] btrfs: add a sanity check for btrfs root in btrfs_next_old_leaf()

[PATCH] btrfs: add a sanity check for btrfs root in btrfs_next_old_leaf()

[Ma Ke]

btrfs_next_old_leaf() doesn't check if the target root is NULL or not,
resulting the null-ptr-deref. Add sanity check for btrfs root before
using it in btrfs_next_old_leaf().

Found by code review.

Cc: stable@vger.kernel.org
Fixes: d96b34248c2f ("btrfs: make send work with concurrent block group relocation")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 fs/btrfs/ctree.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 4e2e1c38d33a..1a3fc3863860 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4794,13 +4794,17 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 	int level;
 	struct extent_buffer *c;
 	struct extent_buffer *next;
-	struct btrfs_fs_info *fs_info = root->fs_info;
+	struct btrfs_fs_info *fs_info;
 	struct btrfs_key key;
 	bool need_commit_sem = false;
 	u32 nritems;
 	int ret;
 	int i;
 
+	if (!root)
+		return -EINVAL;
+
+	fs_info = root->fs_info;
 	/*
 	 * The nowait semantics are used only for write paths, where we don't
 	 * use the tree mod log and sequence numbers.
-- 
2.25.1

Re: [PATCH] btrfs: add a sanity check for btrfs root in btrfs_next_old_leaf()

[Qu Wenruo]

在 2025/2/24 18:29, Ma Ke 写道:
> btrfs_next_old_leaf() doesn't check if the target root is NULL or not,
> resulting the null-ptr-deref. Add sanity check for btrfs root before
> using it in btrfs_next_old_leaf().

Please provide a real world call trace when this is triggered.

There is a prerequisite, the extent tree can only be NULL if
rescue=ibadroots is provided and the extent root is corrupted.

And "rescue=" mount options can only be specified for a fully read-only
fs (no internal log replay or any other thing to write even a bit into
the fs).

Previously read-only scrub can still be triggered on such fs, but
6aecd91a5c5b ("btrfs: avoid NULL pointer dereference if no valid extent
tree") fixed that.

And if you hit such a case in real world, please provide the call trace
so that we know we're not missing some critical situations that extent
tree is accessed for read-only operations.

Thanks,
Qu

>
> Found by code review.
>
> Cc: stable@vger.kernel.org
> Fixes: d96b34248c2f ("btrfs: make send work with concurrent block group relocation")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
>   fs/btrfs/ctree.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 4e2e1c38d33a..1a3fc3863860 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -4794,13 +4794,17 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>   	int level;
>   	struct extent_buffer *c;
>   	struct extent_buffer *next;
> -	struct btrfs_fs_info *fs_info = root->fs_info;
> +	struct btrfs_fs_info *fs_info;
>   	struct btrfs_key key;
>   	bool need_commit_sem = false;
>   	u32 nritems;
>   	int ret;
>   	int i;
>
> +	if (!root)
> +		return -EINVAL;
> +
> +	fs_info = root->fs_info;
>   	/*
>   	 * The nowait semantics are used only for write paths, where we don't
>   	 * use the tree mod log and sequence numbers.

Re: [PATCH] btrfs: add a sanity check for btrfs root in btrfs_next_old_leaf()

[David Sterba]

On Mon, Feb 24, 2025 at 06:48:19PM +1030, Qu Wenruo wrote:
> 在 2025/2/24 18:29, Ma Ke 写道:
> > btrfs_next_old_leaf() doesn't check if the target root is NULL or not,
> > resulting the null-ptr-deref. Add sanity check for btrfs root before
> > using it in btrfs_next_old_leaf().
> 
> Please provide a real world call trace when this is triggered.
> 
> There is a prerequisite, the extent tree can only be NULL if
> rescue=ibadroots is provided and the extent root is corrupted.
> 
> And "rescue=" mount options can only be specified for a fully read-only
> fs (no internal log replay or any other thing to write even a bit into
> the fs).
> 
> Previously read-only scrub can still be triggered on such fs, but
> 6aecd91a5c5b ("btrfs: avoid NULL pointer dereference if no valid extent
> tree") fixed that.
> 
> And if you hit such a case in real world, please provide the call trace
> so that we know we're not missing some critical situations that extent
> tree is accessed for read-only operations.

Agreed, this needs a real way to trigger it. Some pointers do not have
to be checked for NULL because it's guaranteed by some of the caller
higher up in the call chain.

Before we added the rescue options, the invariants were that the extent,
checksum, fs_tree etc always exist when passed as pointers. The example
fix 6aecd91a5c5b show it could be possible under some circumstances. So
if a fix is needed btrfs_next_old_leaf() we also need to see how it
could happen.