[PATCH 5.10 PATCH 1/2] btrfs: check return value of btrfs_commit_transaction in relocation
From: Josef Bacik josef@toxicpanda.com
commit fb686c6824dd6294ca772b92424b8fba666e7d00 upstream
There are a few places where we don't check the return value of btrfs_commit_transaction in relocation.c. Thankfully all these places have straightforward error handling, so simply change all of the sites at once.
Reviewed-by: Qu Wenruo wqu@suse.com Signed-off-by: Josef Bacik josef@toxicpanda.com Reviewed-by: David Sterba dsterba@suse.com Signed-off-by: David Sterba dsterba@suse.com Signed-off-by: Stefan Ghinea stefan.ghinea@windriver.com --- fs/btrfs/relocation.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index c21545c5b34b..7d64180fec2e 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -1895,7 +1895,7 @@ int prepare_to_merge(struct reloc_control *rc, int err) list_splice(&reloc_roots, &rc->reloc_roots);
if (!err) - btrfs_commit_transaction(trans); + err = btrfs_commit_transaction(trans); else btrfs_end_transaction(trans); return err; @@ -3270,8 +3270,7 @@ int prepare_to_relocate(struct reloc_control *rc) */ return PTR_ERR(trans); } - btrfs_commit_transaction(trans); - return 0; + return btrfs_commit_transaction(trans); }
static noinline_for_stack int relocate_block_group(struct reloc_control *rc) @@ -3443,7 +3442,9 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc) err = PTR_ERR(trans); goto out_free; } - btrfs_commit_transaction(trans); + ret = btrfs_commit_transaction(trans); + if (ret && !err) + err = ret; out_free: ret = clean_dirty_subvols(rc); if (ret < 0 && !err)
From: Zixuan Fu r33s3n6@gmail.com
commit 85f02d6c856b9f3a0acf5219de6e32f58b9778eb upstream
In btrfs_relocate_block_group(), the rc is allocated. Then btrfs_relocate_block_group() calls
relocate_block_group() prepare_to_relocate() set_reloc_control()
that assigns rc to the variable fs_info->reloc_ctl. When prepare_to_relocate() returns, it calls
btrfs_commit_transaction() btrfs_start_dirty_block_groups() btrfs_alloc_path() kmem_cache_zalloc()
which may fail for example (or other errors could happen). When the failure occurs, btrfs_relocate_block_group() detects the error and frees rc and doesn't set fs_info->reloc_ctl to NULL. After that, in btrfs_init_reloc_root(), rc is retrieved from fs_info->reloc_ctl and then used, which may cause a use-after-free bug.
This possible bug can be triggered by calling btrfs_ioctl_balance() before calling btrfs_ioctl_defrag().
To fix this possible bug, in prepare_to_relocate(), check if btrfs_commit_transaction() fails. If the failure occurs, unset_reloc_control() is called to set fs_info->reloc_ctl to NULL.
The error log in our fault-injection testing is shown as follows:
[ 58.751070] BUG: KASAN: use-after-free in btrfs_init_reloc_root+0x7ca/0x920 [btrfs] ... [ 58.753577] Call Trace: ... [ 58.755800] kasan_report+0x45/0x60 [ 58.756066] btrfs_init_reloc_root+0x7ca/0x920 [btrfs] [ 58.757304] record_root_in_trans+0x792/0xa10 [btrfs] [ 58.757748] btrfs_record_root_in_trans+0x463/0x4f0 [btrfs] [ 58.758231] start_transaction+0x896/0x2950 [btrfs] [ 58.758661] btrfs_defrag_root+0x250/0xc00 [btrfs] [ 58.759083] btrfs_ioctl_defrag+0x467/0xa00 [btrfs] [ 58.759513] btrfs_ioctl+0x3c95/0x114e0 [btrfs] ... [ 58.768510] Allocated by task 23683: [ 58.768777] ____kasan_kmalloc+0xb5/0xf0 [ 58.769069] __kmalloc+0x227/0x3d0 [ 58.769325] alloc_reloc_control+0x10a/0x3d0 [btrfs] [ 58.769755] btrfs_relocate_block_group+0x7aa/0x1e20 [btrfs] [ 58.770228] btrfs_relocate_chunk+0xf1/0x760 [btrfs] [ 58.770655] __btrfs_balance+0x1326/0x1f10 [btrfs] [ 58.771071] btrfs_balance+0x3150/0x3d30 [btrfs] [ 58.771472] btrfs_ioctl_balance+0xd84/0x1410 [btrfs] [ 58.771902] btrfs_ioctl+0x4caa/0x114e0 [btrfs] ... [ 58.773337] Freed by task 23683: ... [ 58.774815] kfree+0xda/0x2b0 [ 58.775038] free_reloc_control+0x1d6/0x220 [btrfs] [ 58.775465] btrfs_relocate_block_group+0x115c/0x1e20 [btrfs] [ 58.775944] btrfs_relocate_chunk+0xf1/0x760 [btrfs] [ 58.776369] __btrfs_balance+0x1326/0x1f10 [btrfs] [ 58.776784] btrfs_balance+0x3150/0x3d30 [btrfs] [ 58.777185] btrfs_ioctl_balance+0xd84/0x1410 [btrfs] [ 58.777621] btrfs_ioctl+0x4caa/0x114e0 [btrfs] ...
Reported-by: TOTE Robot oslab@tsinghua.edu.cn CC: stable@vger.kernel.org # 5.15+ Reviewed-by: Sweet Tea Dorminy sweettea-kernel@dorminy.me Reviewed-by: Nikolay Borisov nborisov@suse.com Signed-off-by: Zixuan Fu r33s3n6@gmail.com Signed-off-by: David Sterba dsterba@suse.com Signed-off-by: Stefan Ghinea stefan.ghinea@windriver.com --- fs/btrfs/relocation.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 7d64180fec2e..93db4486a943 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -3270,7 +3270,12 @@ int prepare_to_relocate(struct reloc_control *rc) */ return PTR_ERR(trans); } - return btrfs_commit_transaction(trans); + + ret = btrfs_commit_transaction(trans); + if (ret) + unset_reloc_control(rc); + + return ret; }
static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
On Fri, Jun 09, 2023 at 12:19:58AM +0300, Stefan Ghinea wrote:
From: Josef Bacik josef@toxicpanda.com
commit fb686c6824dd6294ca772b92424b8fba666e7d00 upstream
There are a few places where we don't check the return value of btrfs_commit_transaction in relocation.c. Thankfully all these places have straightforward error handling, so simply change all of the sites at once.
Reviewed-by: Qu Wenruo wqu@suse.com Signed-off-by: Josef Bacik josef@toxicpanda.com Reviewed-by: David Sterba dsterba@suse.com Signed-off-by: David Sterba dsterba@suse.com Signed-off-by: Stefan Ghinea stefan.ghinea@windriver.com
fs/btrfs/relocation.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
All backports now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Stefan Ghinea