| Message ID | 9cdf58c2f045863e98a52d7f9d5102ba12b87f07.1687496547.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix race between balance and cancel/pause | expand |
On Fri, Jun 23, 2023 at 01:05:41AM -0400, Josef Bacik wrote: > Syzbot reported a panic that looks like this > > assertion failed: fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED, in fs/btrfs/ioctl.c:465 > ------------[ cut here ]------------ > kernel BUG at fs/btrfs/messages.c:259! > RIP: 0010:btrfs_assertfail+0x2c/0x30 fs/btrfs/messages.c:259 > Call Trace: > <TASK> > btrfs_exclop_balance fs/btrfs/ioctl.c:465 [inline] > btrfs_ioctl_balance fs/btrfs/ioctl.c:3564 [inline] > btrfs_ioctl+0x531e/0x5b30 fs/btrfs/ioctl.c:4632 > vfs_ioctl fs/ioctl.c:51 [inline] > __do_sys_ioctl fs/ioctl.c:870 [inline] > __se_sys_ioctl fs/ioctl.c:856 [inline] > __x64_sys_ioctl+0x197/0x210 fs/ioctl.c:856 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > The reproducer is running a balance and a cancel or pause in parallel. The way > balance finishes is a bit wonky, if we were paused we need to save the > balance_ctl in the fs_info, but clear it otherwise and cleanup. However > we rely on the return values being specific errors, or having a cancel > request or no pause request. If balance completes and returns 0, but we > have a pause or cancel request we won't do the appropriate cleanup, and > then the next time we try to start a balance we'll trip this ASSERT. > > The error handling is just wrong here, we always want to clean up, > unless we got -ECANCELLED and we set the appropriate pause flag in the > exclusive op. With this patch the reproducer ran for an hour without > tripping, previously it would trip in less than a few minutes. > > Reported-by: syzbot+c0f3acf145cb465426d5@syzkaller.appspotmail.com > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Added to misc-next, thanks.
I think this patch does not fully fix the issue. This patch just fix assertion panic, but in the race situation, the ioctl pause request still returns an incorrect value 0 to the user which mislead the user the pause request finished successfully. In fact, the balance request has not been paused. Test results and analysis are as follows: https://lore.kernel.org/linux-btrfs/20230726030617.109018-1-xiaoshoukui@gmail.com/T/#me125d17fa59e9e671149cc76d410ced747f488b1
On Mon, Aug 07, 2023 at 10:47:48PM -0400, xiaoshoukui wrote: > I think this patch does not fully fix the issue. > > This patch just fix assertion panic, but in the race situation, the ioctl pause > request still returns an incorrect value 0 to the user which mislead the user the > pause request finished successfully. In fact, the balance request has not been paused. > > Test results and analysis are as follows: > https://lore.kernel.org/linux-btrfs/20230726030617.109018-1-xiaoshoukui@gmail.com/T/#me125d17fa59e9e671149cc76d410ced747f488b1 They're just two different issues. My patch is concerned with the panic, yours is concerned with getting the correct return value out to the user. Rebase your patch ontop of Sterba's tree with my fix and send it along, getting an accurate errno out to the user is a reasonable goal. Thanks, Josef
> They're just two different issues. My patch is concerned with the panic, yours > is concerned with getting the correct return value out to the user. Agreed. > Rebase your patch ontop of Sterba's tree with my fix and send it along, getting > an accurate errno out to the user is a reasonable goal. Thanks, Send the patch through below thread, pls review. Thanks. https://lore.kernel.org/linux-btrfs/20230810034810.23934-1-xiaoshoukui@gmail.com/T/#u
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index a536d0e0e055..90d25b2a6fd1 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -4081,14 +4081,6 @@ static int alloc_profile_is_valid(u64 flags, int extended) return has_single_bit_set(flags); } -static inline int balance_need_close(struct btrfs_fs_info *fs_info) -{ - /* cancel requested || normal exit path */ - return atomic_read(&fs_info->balance_cancel_req) || - (atomic_read(&fs_info->balance_pause_req) == 0 && - atomic_read(&fs_info->balance_cancel_req) == 0); -} - /* * Validate target profile against allowed profiles and return true if it's OK. * Otherwise print the error message and return false. @@ -4278,6 +4270,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info, u64 num_devices; unsigned seq; bool reducing_redundancy; + bool paused = false; int i; if (btrfs_fs_closing(fs_info) || @@ -4408,6 +4401,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info, if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req)) { btrfs_info(fs_info, "balance: paused"); btrfs_exclop_balance(fs_info, BTRFS_EXCLOP_BALANCE_PAUSED); + paused = true; } /* * Balance can be canceled by: @@ -4436,8 +4430,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info, btrfs_update_ioctl_balance_args(fs_info, bargs); } - if ((ret && ret != -ECANCELED && ret != -ENOSPC) || - balance_need_close(fs_info)) { + /* We didn't pause, we can clean everything up. */ + if (!paused) { reset_balance_state(fs_info); btrfs_exclop_finish(fs_info); }
Syzbot reported a panic that looks like this assertion failed: fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED, in fs/btrfs/ioctl.c:465 ------------[ cut here ]------------ kernel BUG at fs/btrfs/messages.c:259! RIP: 0010:btrfs_assertfail+0x2c/0x30 fs/btrfs/messages.c:259 Call Trace: <TASK> btrfs_exclop_balance fs/btrfs/ioctl.c:465 [inline] btrfs_ioctl_balance fs/btrfs/ioctl.c:3564 [inline] btrfs_ioctl+0x531e/0x5b30 fs/btrfs/ioctl.c:4632 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:870 [inline] __se_sys_ioctl fs/ioctl.c:856 [inline] __x64_sys_ioctl+0x197/0x210 fs/ioctl.c:856 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd The reproducer is running a balance and a cancel or pause in parallel. The way balance finishes is a bit wonky, if we were paused we need to save the balance_ctl in the fs_info, but clear it otherwise and cleanup. However we rely on the return values being specific errors, or having a cancel request or no pause request. If balance completes and returns 0, but we have a pause or cancel request we won't do the appropriate cleanup, and then the next time we try to start a balance we'll trip this ASSERT. The error handling is just wrong here, we always want to clean up, unless we got -ECANCELLED and we set the appropriate pause flag in the exclusive op. With this patch the reproducer ran for an hour without tripping, previously it would trip in less than a few minutes. Reported-by: syzbot+c0f3acf145cb465426d5@syzkaller.appspotmail.com Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/volumes.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)