[PATCH] btrfs: add missing NULL check in btrfs_free_tree_block()

[PATCH] btrfs: add missing NULL check in btrfs_free_tree_block()

[Riyan Dhiman]

In commit 3ba2d3648f9dc (btrfs: reflow btrfs_free_tree_block), the block
group lookup using btrfs_lookup_block_group() was added in btrfs_free_tree_block().
However, the return value of this function is not checked for a NULL result,
which can lead to null pointer dereferences if the block group is not found.

This patch adds a check to ensure that if btrfs_lookup_block_group() returns
NULL, the function will gracefully exit, preventing further operations that
rely on a valid block group pointer.

Signed-off-by: Riyan Dhiman <riyandhiman14@gmail.com>
---
Compile tested only

 fs/btrfs/extent-tree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a5966324607d..7782d4436ca0 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3454,6 +3454,8 @@ int btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 	}
 
 	bg = btrfs_lookup_block_group(fs_info, buf->start);
+	if (!bg)
+		goto out;
 
 	if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
 		pin_down_extent(trans, bg, buf->start, buf->len, 1);
-- 
2.46.1

Re: [PATCH] btrfs: add missing NULL check in btrfs_free_tree_block()

[Filipe Manana]

On Thu, Sep 26, 2024 at 8:50 AM Riyan Dhiman <riyandhiman14@gmail.com> wrote:
>
> In commit 3ba2d3648f9dc (btrfs: reflow btrfs_free_tree_block), the block
> group lookup using btrfs_lookup_block_group() was added in btrfs_free_tree_block().
> However, the return value of this function is not checked for a NULL result,
> which can lead to null pointer dereferences if the block group is not found.
>
> This patch adds a check to ensure that if btrfs_lookup_block_group() returns
> NULL, the function will gracefully exit, preventing further operations that
> rely on a valid block group pointer.
>
> Signed-off-by: Riyan Dhiman <riyandhiman14@gmail.com>
> ---
> Compile tested only
>
>  fs/btrfs/extent-tree.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index a5966324607d..7782d4436ca0 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3454,6 +3454,8 @@ int btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>         }
>
>         bg = btrfs_lookup_block_group(fs_info, buf->start);
> +       if (!bg)
> +               goto out;

We are supposed to be able to always find the block group to which the
extent buffer belongs to.
If we don't, it means we have a serious corruption or bug.

If that happens we want it to be noisy so that it gets reported and we
look at it.
Letting a NULL pointer dereference happen is one way of getting our attention.

O more gentle and explicit way would be to have a:    ASSERT(bg != NULL);

But certainly not ignoring the problem.

Thanks.

>
>         if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>                 pin_down_extent(trans, bg, buf->start, buf->len, 1);
> --
> 2.46.1
>
>

[PATCH] btrfs: add missing NULL check in btrfs_free_tree_block()

[Riyan Dhiman]

> If that happens we want it to be noisy so that it gets reported and we
> look at it.
> Letting a NULL pointer dereference happen is one way of getting our attention.
>
> O more gentle and explicit way would be to have a:    ASSERT(bg != NULL);

I am wondering whether it would be better to have an ASSERT statement here, as you 
suggested, or use a BUG_ON instead.

I haven't personally encountered a null pointer dereference issue in a live kernel 
environment, so I'm not sure how the kernel behaves in such a scenario. However, it 
seems wrong to leave it unhandled as it currently is.

Regards,
Riyan Dhiman

Re: [PATCH] btrfs: add missing NULL check in btrfs_free_tree_block()

[Filipe Manana]

On Thu, Sep 26, 2024 at 2:57 PM Riyan Dhiman <riyandhiman14@gmail.com> wrote:
>
> > If that happens we want it to be noisy so that it gets reported and we
> > look at it.
> > Letting a NULL pointer dereference happen is one way of getting our attention.
> >
> > O more gentle and explicit way would be to have a:    ASSERT(bg != NULL);
>
> I am wondering whether it would be better to have an ASSERT statement here, as you
> suggested, or use a BUG_ON instead.

Please no, we're trying to get rid of all BUG_ON()s in the code base,
and replace them either with proper error handling or an ASSERT, or
both sometimes since CONFIG_BTRFS_ASSERT is not enabled by default in
some distros (we say in kconfig that it's meant only for developers).

>
> I haven't personally encountered a null pointer dereference issue in a live kernel
> environment, so I'm not sure how the kernel behaves in such a scenario. However, it
> seems wrong to leave it unhandled as it currently is.

Just add a:

if (WARN_ON(!bg)) {
    btrfs_abort_transaction(trans, -ENOENT);
    btrfs_err(fs_info, "block group not found for extent buffer %llu
generation %llu root %llu transaction %llu",
                   buf->start, btrfs_header_generation(buf), root_id,
trans->transid);
    return -ENOENT;
}

Thanks.

>
> Regards,
> Riyan Dhiman