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