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

[PATCH v2] 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>
---
v2: Added WARN_ON if block group is NULL instead of jump to out block.
v1: if block group is NULL, jump to out block.

Compile tested only

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a5966324607d..be031be3dfe5 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3455,6 +3455,13 @@ int btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 
 	bg = btrfs_lookup_block_group(fs_info, buf->start);
 
+	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;
+	}
 	if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
 		pin_down_extent(trans, bg, buf->start, buf->len, 1);
 		btrfs_put_block_group(bg);
-- 
2.46.1

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

[Filipe Manana]

On Thu, Sep 26, 2024 at 4:06 PM 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().

It wasn't, it was just a refactoring and the variable name was renamed
from "cache" to "bg".
The whole logic of looking up for a block group and ignoring NULL,
because it should never happen, comes from:

commit f0486c68e4bd9a06a5904d3eeb3a0d73a83befb8
Author: Yan, Zheng <zheng.yan@oracle.com>
Date:   Sun May 16 10:46:25 2010 -0400

    Btrfs: Introduce contexts for metadata reservation

You have to follow the git history and not blame only the most recent
commit touching a line using git blame.


> 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.

Just add that this should be impossible to happen, because if we are
freeing an extent buffer it means there's a block group to which it
belongs.

>
> 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>
> ---
> v2: Added WARN_ON if block group is NULL instead of jump to out block.
> v1: if block group is NULL, jump to out block.
>
> Compile tested only
>
>  fs/btrfs/extent-tree.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index a5966324607d..be031be3dfe5 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3455,6 +3455,13 @@ int btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>
>         bg = btrfs_lookup_block_group(fs_info, buf->start);
>
> +       if (WARN_ON(!bg)) {

Sorry I misled you here.
We don't need the WARN_ON() because btrfs_abort_transaction() already
produces a trace, and I forgot that before.

> +           btrfs_abort_transaction(trans, -ENOENT);
> +           btrfs_err(fs_info, "block group not found for extent buffer %llu generation %llu root %llu transaction %llu",

As the line is long, move the string argument to a new line.

Everything else looks fine, thanks.

> +                               buf->start, btrfs_header_generation(buf), root_id,
> +                               trans->transid);
> +           return -ENOENT;
> +       }
>         if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>                 pin_down_extent(trans, bg, buf->start, buf->len, 1);
>                 btrfs_put_block_group(bg);
> --
> 2.46.1
>
>