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@xxxxxxxxx> 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