linux-kernel - Re: [PATCH] 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: <CAL3q7H4prrKHKP_Ls-eYhiHtp4x84+d2W189eV4R+KF4V89yGA@mail.gmail.com>
Date: Thu, 26 Sep 2024 15:09:16 +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] btrfs: add missing NULL check in btrfs_free_tree_block()

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