| Message ID | 20220503083637.1051023-3-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor btrfs_ioctl_balance | expand |
Hi Nikolay,
url: https://github.com/intel-lab-lkp/linux/commits/Nikolay-Borisov/Refactor-btrfs_ioctl_balance/20220503-163837
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-m021-20220502 (https://download.01.org/0day-ci/archive/20220504/202205041423.NVVJIHSj-lkp@intel.com/config )
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
fs/btrfs/ioctl.c:4493 btrfs_ioctl_balance() error: double free of 'bargs'
vim +/bargs +4493 fs/btrfs/ioctl.c
c9e9f97bdfb64d Ilya Dryomov 2012-01-16 4484
0f89abf56abbd0 Christian Engelmayer 2015-10-21 4485 kfree(bctl);
ed0fb78fb6aa29 Ilya Dryomov 2013-01-20 4486 out_unlock:
c9e9f97bdfb64d Ilya Dryomov 2012-01-16 4487 mutex_unlock(&fs_info->balance_mutex);
ed0fb78fb6aa29 Ilya Dryomov 2013-01-20 4488 if (need_unlock)
c3e1f96c37d0f8 Goldwyn Rodrigues 2020-08-25 4489 btrfs_exclop_finish(fs_info);
ed0fb78fb6aa29 Ilya Dryomov 2013-01-20 4490 out:
c696e46e6ec2b3 Nikolay Borisov 2022-05-03 4491 kfree(bargs);
^^^^^
e54bfa31044d60 Liu Bo 2012-06-29 4492 mnt_drop_write_file(file);
c746db1b6ed99f Nikolay Borisov 2022-03-30 @4493 kfree(bargs);
^^^^^
Freed twice.
c9e9f97bdfb64d Ilya Dryomov 2012-01-16 4494 return ret;
c9e9f97bdfb64d Ilya Dryomov 2012-01-16 4495 }
On 4.05.22 г. 9:46 ч., Dan Carpenter wrote: > Hi Nikolay, > > url: https://github.com/intel-lab-lkp/linux/commits/Nikolay-Borisov/Refactor-btrfs_ioctl_balance/20220503-163837 > base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next > config: i386-randconfig-m021-20220502 (https://download.01.org/0day-ci/archive/20220504/202205041423.NVVJIHSj-lkp@intel.com/config ) > compiler: gcc-11 (Debian 11.2.0-20) 11.2.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > smatch warnings: > fs/btrfs/ioctl.c:4493 btrfs_ioctl_balance() error: double free of 'bargs' > > vim +/bargs +4493 fs/btrfs/ioctl.c Yep, a genuine braino, the fix is to remove either of the calls. I personally would go with the one before mnt_drop_write so that we drop the write ASAP but in the end I doubt it makes any practical difference. David care to fold this or shall I resend? > > c9e9f97bdfb64d Ilya Dryomov 2012-01-16 4484 > 0f89abf56abbd0 Christian Engelmayer 2015-10-21 4485 kfree(bctl); > ed0fb78fb6aa29 Ilya Dryomov 2013-01-20 4486 out_unlock: > c9e9f97bdfb64d Ilya Dryomov 2012-01-16 4487 mutex_unlock(&fs_info->balance_mutex); > ed0fb78fb6aa29 Ilya Dryomov 2013-01-20 4488 if (need_unlock) > c3e1f96c37d0f8 Goldwyn Rodrigues 2020-08-25 4489 btrfs_exclop_finish(fs_info); > ed0fb78fb6aa29 Ilya Dryomov 2013-01-20 4490 out: > c696e46e6ec2b3 Nikolay Borisov 2022-05-03 4491 kfree(bargs); > ^^^^^ > > e54bfa31044d60 Liu Bo 2012-06-29 4492 mnt_drop_write_file(file); > c746db1b6ed99f Nikolay Borisov 2022-03-30 @4493 kfree(bargs); > ^^^^^ > > Freed twice. > > c9e9f97bdfb64d Ilya Dryomov 2012-01-16 4494 return ret; > c9e9f97bdfb64d Ilya Dryomov 2012-01-16 4495 } >
On Wed, May 04, 2022 at 11:44:51AM +0300, Nikolay Borisov wrote: > > > On 4.05.22 г. 9:46 ч., Dan Carpenter wrote: > > Hi Nikolay, > > > > url: https://github.com/intel-lab-lkp/linux/commits/Nikolay-Borisov/Refactor-btrfs_ioctl_balance/20220503-163837 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next > > config: i386-randconfig-m021-20220502 (https://download.01.org/0day-ci/archive/20220504/202205041423.NVVJIHSj-lkp@intel.com/config ) > > compiler: gcc-11 (Debian 11.2.0-20) 11.2.0 > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > smatch warnings: > > fs/btrfs/ioctl.c:4493 btrfs_ioctl_balance() error: double free of 'bargs' > > > > vim +/bargs +4493 fs/btrfs/ioctl.c > > > Yep, a genuine braino, the fix is to remove either of the calls. I > personally would go with the one before mnt_drop_write so that we drop > the write ASAP but in the end I doubt it makes any practical difference. > David care to fold this or shall I resend? Please resend, thanks.
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: c696e46e6ec2b391d6e350b4323ef7e7bafa7bca ("[PATCH 2/2] btrfs: Use btrfs_try_lock_balance in btrfs_ioctl_balance")
url: https://github.com/intel-lab-lkp/linux/commits/Nikolay-Borisov/Refactor-btrfs_ioctl_balance/20220503-163837
base: https://git.kernel.org/cgit/linux/kernel/git/kdave/linux.git for-next
patch link: https://lore.kernel.org/linux-btrfs/20220503083637.1051023-3-nborisov@suse.com
in testcase: xfstests
version: xfstests-x86_64-46e1b83-1_20220414
with following parameters:
disk: 6HDD
fs: btrfs
test: btrfs-group-04
ucode: 0x28
test-description: xfstests is a regression test suite for xfs and other files ystems.
test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
on test machine: 8 threads 1 sockets Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz with 8G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <oliver.sang@intel.com>
[ 72.075848][ T6752] BTRFS info (device sda2): balance: paused
[ 72.081574][ T6752] ==================================================================
[ 72.089422][ T6752] BUG: KASAN: double-free or invalid-free in btrfs_ioctl_balance+0x2d6/0x600 [btrfs]
[ 72.098720][ T6752]
[ 72.100885][ T6752] CPU: 0 PID: 6752 Comm: btrfs Not tainted 5.18.0-rc4-00382-gc696e46e6ec2 #1
[ 72.109425][ T6752] Hardware name: Dell Inc. OptiPlex 9020/0DNKMN, BIOS A05 12/05/2013
[ 72.117278][ T6752] Call Trace:
[ 72.120393][ T6752] <TASK>
[ 72.123164][ T6752] ? btrfs_ioctl_balance+0x2d6/0x600 [btrfs]
[ 72.129004][ T6752] dump_stack_lvl+0x34/0x44
[ 72.133326][ T6752] print_address_description+0x1f/0x200
[ 72.133821][ T6763] BTRFS warning (device sda2): cannot activate swapfile while exclusive operation is running
[ 72.139714][ T6752] ? btrfs_ioctl_balance+0x2d6/0x600 [btrfs]
[ 72.155412][ T6752] print_report.cold+0x55/0x22c
[ 72.160078][ T6752] ? _raw_spin_lock_irqsave+0x87/0x100
[ 72.165343][ T6752] ? btrfs_ioctl_balance+0x2d6/0x600 [btrfs]
[ 72.171179][ T6752] ? btrfs_ioctl_balance+0x2d6/0x600 [btrfs]
[ 72.177002][ T6752] kasan_report_invalid_free+0x79/0x100
[ 72.182354][ T6752] ? __kasan_kmalloc+0x80/0xc0
[ 72.186929][ T6752] ? btrfs_ioctl_balance+0x2d6/0x600 [btrfs]
[ 72.192753][ T6752] __kasan_slab_free+0x152/0x180
[ 72.197499][ T6752] kfree+0xa6/0x340
[ 72.201129][ T6752] btrfs_ioctl_balance+0x2d6/0x600 [btrfs]
[ 72.206780][ T6752] btrfs_ioctl+0x893/0x2580 [btrfs]
[ 72.211831][ T6752] ? btrfs_ioctl_get_supported_features+0x40/0x40 [btrfs]
[ 72.218772][ T6752] ? vma_link+0x4e8/0x900
[ 72.222920][ T6752] ? handle_mm_fault+0x1c7/0x640
[ 72.227673][ T6752] ? up_read+0x15/0xc0
[ 72.231560][ T6752] __x64_sys_ioctl+0x127/0x1c0
[ 72.236136][ T6752] do_syscall_64+0x3b/0xc0
[ 72.240369][ T6752] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 72.246063][ T6752] RIP: 0033:0x7febcb4af427
[ 72.250294][ T6752] Code: 00 00 90 48 8b 05 69 aa 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 39 aa 0c 00 f7 d8 64 89 01 48
[ 72.269589][ T6752] RSP: 002b:00007ffde9257118 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
[ 72.277781][ T6752] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007febcb4af427
[ 72.285542][ T6752] RDX: 00007ffde92571a8 RSI: 00000000c4009420 RDI: 0000000000000004
[ 72.293302][ T6752] RBP: 00007ffde92571a8 R08: 0000000000000003 R09: 0000000000000078
[ 72.301060][ T6752] R10: fffffffffffffa4a R11: 0000000000000206 R12: 0000000000000004
[ 72.308819][ T6752] R13: 00007ffde92598b8 R14: 0000000000000002 R15: 0000000000000000
[ 72.316579][ T6752] </TASK>
[ 72.319432][ T6752]
[ 72.321597][ T6752] Allocated by task 3586:
[ 72.325741][ T6752] kasan_save_stack+0x1e/0x40
[ 72.330232][ T6752] __kasan_kmalloc+0x81/0xc0
[ 72.334636][ T6752] load_elf_phdrs+0xd9/0x1c0
[ 72.339039][ T6752] load_elf_binary+0x194/0x2700
[ 72.343707][ T6752] search_binary_handler+0x18d/0x480
[ 72.348811][ T6752] exec_binprm+0xd6/0x440
[ 72.352954][ T6752] bprm_execve+0x485/0x840
[ 72.357790][ T6752] do_execveat_common+0x4c7/0x680
[ 72.363226][ T6752] __x64_sys_execve+0x88/0xc0
[ 72.367719][ T6752] do_syscall_64+0x3b/0xc0
[ 72.371950][ T6752] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 72.377643][ T6752]
[ 72.379805][ T6752] Freed by task 6752:
[ 72.383605][ T6752] kasan_save_stack+0x1e/0x40
[ 72.388096][ T6752] kasan_set_track+0x21/0x40
[ 72.392499][ T6752] kasan_set_free_info+0x20/0x40
[ 72.397248][ T6752] __kasan_slab_free+0x108/0x180
[ 72.401994][ T6752] kfree+0xa6/0x340
[ 72.405621][ T6752] btrfs_ioctl_balance+0x2c4/0x600 [btrfs]
[ 72.411298][ T6752] btrfs_ioctl+0x893/0x2580 [btrfs]
[ 72.416347][ T6752] __x64_sys_ioctl+0x127/0x1c0
[ 72.420923][ T6752] do_syscall_64+0x3b/0xc0
[ 72.425155][ T6752] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 72.430848][ T6752]
[ 72.433012][ T6752] The buggy address belongs to the object at ffff88811a7ee000
[ 72.433012][ T6752] which belongs to the cache kmalloc-1k of size 1024
[ 72.446798][ T6752] The buggy address is located 0 bytes inside of
[ 72.446798][ T6752] 1024-byte region [ffff88811a7ee000, ffff88811a7ee400)
[ 72.459724][ T6752]
[ 72.461890][ T6752] The buggy address belongs to the physical page:
[ 72.468099][ T6752] page:00000000a7668fa1 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x11a7e8
[ 72.478097][ T6752] head:00000000a7668fa1 order:3 compound_mapcount:0 compound_pincount:0
[ 72.486200][ T6752] flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
[ 72.494221][ T6752] raw: 0017ffffc0010200 ffffea0008552200 dead000000000002 ffff888100042dc0
[ 72.502583][ T6752] raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
[ 72.510944][ T6752] page dumped because: kasan: bad access detected
[ 72.517154][ T6752]
[ 72.519316][ T6752] Memory state around the buggy address:
[ 72.524750][ T6752] ffff88811a7edf00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 72.532596][ T6752] ffff88811a7edf80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 72.540443][ T6752] >ffff88811a7ee000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 72.548288][ T6752] ^
[ 72.552173][ T6752] ffff88811a7ee080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 72.560018][ T6752] ffff88811a7ee100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 72.567863][ T6752] ==================================================================
[ 72.575771][ T6752] Disabling lock debugging due to kernel taint
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 8e0cc17d5f81..f2cfbce9bec4 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4406,7 +4406,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg) struct btrfs_fs_info *fs_info = root->fs_info; struct btrfs_ioctl_balance_args *bargs; struct btrfs_balance_control *bctl; - bool need_unlock; /* for mut. excl. ops lock */ + bool need_unlock = true; /* for mut. excl. ops lock */ int ret; if (!capable(CAP_SYS_ADMIN)) @@ -4423,53 +4423,12 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg) goto out; } -again: - if (btrfs_exclop_start(fs_info, BTRFS_EXCLOP_BALANCE)) { - mutex_lock(&fs_info->balance_mutex); - need_unlock = true; - goto locked; - } - - /* - * mut. excl. ops lock is locked. Three possibilities: - * (1) some other op is running - * (2) balance is running - * (3) balance is paused -- special case (think resume) - */ - mutex_lock(&fs_info->balance_mutex); - if (fs_info->balance_ctl) { - /* this is either (2) or (3) */ - if (!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) { - mutex_unlock(&fs_info->balance_mutex); - /* - * Lock released to allow other waiters to continue, - * we'll reexamine the status again. - */ - mutex_lock(&fs_info->balance_mutex); - - if (fs_info->balance_ctl && - !test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) { - /* this is (3) */ - need_unlock = false; - goto locked; - } - - mutex_unlock(&fs_info->balance_mutex); - goto again; - } else { - /* this is (2) */ - mutex_unlock(&fs_info->balance_mutex); - ret = -EINPROGRESS; - goto out; - } - } else { - /* this is (1) */ - mutex_unlock(&fs_info->balance_mutex); - ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS; + ret = btrfs_try_lock_balance(fs_info, &need_unlock); + if (ret) goto out; - } -locked: + lockdep_assert_held(&fs_info->balance_mutex); + if (bargs->flags & BTRFS_BALANCE_RESUME) { if (!fs_info->balance_ctl) { ret = -ENOTCONN; @@ -4529,6 +4488,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg) if (need_unlock) btrfs_exclop_finish(fs_info); out: + kfree(bargs); mnt_drop_write_file(file); kfree(bargs); return ret;
This eliminates 2 labels and makes the code generally more streamlined. Also rename the 'out_bargs' label to 'out_unlock' since bargs is going to be freed under the 'out' label. This also fixes a memory leak since bargs wasn't correctly freed in one of the condition which are now moved in btrfs_try_lock_balance. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/ioctl.c | 52 ++++++------------------------------------------ 1 file changed, 6 insertions(+), 46 deletions(-)