| Message ID | 20240926074947.39415-1-riyandhiman14@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add missing NULL check in btrfs_free_tree_block() | expand |
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 > >
> 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
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
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);
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(+)