linux-kernel - Re: [PATCH v2] btrfs: add missing NULL check in btrfs_free_tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAL3q7H447Sf0__CVSmP==kQkyVOQqDwN6Lb48nZ3iDZB8Nokfw@mail.gmail.com>
Date: Thu, 26 Sep 2024 16:24:47 +0100
From: Filipe Manana <fdmanana@...nel.org>
To: Riyan Dhiman <riyandhiman14@...il.com>
Cc: clm@...com, josef@...icpanda.com, dsterba@...e.com, 
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] btrfs: add missing NULL check in btrfs_free_tree_block()

On Thu, Sep 26, 2024 at 4:06 PM Riyan Dhiman <riyandhiman14@...il.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@...cle.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@...il.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
>
>