| Message ID | 1541946144-8174-9-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix replace-start and replace-cancel racing | expand |
On Sun, Nov 11, 2018 at 10:22:23PM +0800, Anand Jain wrote: > As of now only user requested replace cancel can cancel the replace-scrub > so no need to log error for it. This has probably some user visible effect or threre are steps to reproduce the message even if it's not expected (ie. the case that this patch fixes). Please update the changelog, thanks.
On 11/15/2018 11:31 PM, David Sterba wrote: > On Sun, Nov 11, 2018 at 10:22:23PM +0800, Anand Jain wrote: >> As of now only user requested replace cancel can cancel the replace-scrub >> so no need to log error for it. > > This has probably some user visible effect or threre are steps to > reproduce the message even if it's not expected (ie. the case that this > patch fixes). Please update the changelog, thanks. > before the patch [1] [1] btrfs: fix use-after-free due to race between replace start and cancel We used to set the replace_state to CANCELED [2] and then call btrfs_scrub_cancel(), but the problem with this approach is if the scrub isn't running yet, then things get messier. [2] - dev_replace->replace_state = BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED; So to fix, [1] shall set replace_state to CANCELED only after the replace cancel thread has successfully canceled the replace using btrfs_scrub_cancel(). And about the cleanup process for the replace target.. we call btrfs_dev_replace_finishing(.. ret) after ret= btrfs_scrub_dev() now ret is -ECANCELED due to replace cancel request by the user. (its not set to -ECANCELED for any other reason). btrfs_dev_replace_finishing() has the following code [3] which it earlier blocked processing of the cleanup process after the cancel, because the replace_state was already updated to BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED. [3] -------------- static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, int scrub_ret) { :: /* was the operation canceled, or is it finished? */ if (dev_replace->replace_state != BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED) { btrfs_dev_replace_read_unlock(dev_replace); mutex_unlock(&dev_replace->lock_finishing_cancel_unmount); return 0; } ----------- In fact before this patch [1] the code wasn't sync-ing the IO when replace was canceled. Which this patch also fixes by using the btrfs_dev_replace_finishing() ----------- btrfs_dev_replace_finishing :: /* * flush all outstanding I/O and inode extent mappings before the * copy operation is declared as being finished */ ret = btrfs_start_delalloc_roots(fs_info, -1); if (ret) { mutex_unlock(&dev_replace->lock_finishing_cancel_unmount); return ret; } btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1); ----------- So to answer above question... these warn and error log wasn't reported before when replace was canceled and now since I am using the btrfs_dev_replace_finishing() to finish the job of cancel.. it shall be reported which is ok to quite. Do you think we still need to update the change log? HTH. Thanks, Anand
On 11/16/2018 06:29 PM, Anand Jain wrote: > > > On 11/15/2018 11:31 PM, David Sterba wrote: >> On Sun, Nov 11, 2018 at 10:22:23PM +0800, Anand Jain wrote: >>> As of now only user requested replace cancel can cancel the >>> replace-scrub >>> so no need to log error for it. >> >> This has probably some user visible effect or threre are steps to >> reproduce the message even if it's not expected (ie. the case that this >> patch fixes). Please update the changelog, thanks. >> > > > > before the patch [1] > [1] > btrfs: fix use-after-free due to race between replace start and cancel > > We used to set the replace_state to CANCELED [2] and then call > btrfs_scrub_cancel(), but the problem with this approach is > if the scrub isn't running yet, then things get messier. > > [2] > - dev_replace->replace_state = > BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED; > > So to fix, [1] shall set replace_state to CANCELED only after the > replace cancel thread has successfully canceled the replace using > btrfs_scrub_cancel(). > > And about the cleanup process for the replace target.. > we call > btrfs_dev_replace_finishing(.. ret) > after > ret= btrfs_scrub_dev() > > now ret is -ECANCELED due to replace cancel request by the user. > (its not set to -ECANCELED for any other reason). > > btrfs_dev_replace_finishing() has the following code [3] which it > earlier blocked processing of the cleanup process after the cancel, > because the replace_state was already updated to > BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED. > > > [3] > -------------- > static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, > int scrub_ret) > { > > :: > /* was the operation canceled, or is it finished? */ > if (dev_replace->replace_state != > BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED) { > btrfs_dev_replace_read_unlock(dev_replace); > mutex_unlock(&dev_replace->lock_finishing_cancel_unmount); > return 0; > } > ----------- > > In fact before this patch [1] the code wasn't sync-ing the IO > when replace was canceled. Which this patch also fixes by using > the btrfs_dev_replace_finishing() > > > ----------- > btrfs_dev_replace_finishing > :: > /* > * flush all outstanding I/O and inode extent mappings before the > * copy operation is declared as being finished > */ > ret = btrfs_start_delalloc_roots(fs_info, -1); > if (ret) { > mutex_unlock(&dev_replace->lock_finishing_cancel_unmount); > return ret; > } > btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1); > ----------- > > So to answer above question... these warn and error log wasn't > reported before when replace was canceled and now since I am > using the btrfs_dev_replace_finishing() to finish the job > of cancel.. it shall be reported which is ok to quite. > > Do you think we still need to update the change log? OR I think more appropriately this patch should be merged with [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel -Anand > HTH. > > Thanks, Anand
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c index 9031a362921a..40a0942b4659 100644 --- a/fs/btrfs/dev-replace.c +++ b/fs/btrfs/dev-replace.c @@ -622,7 +622,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, src_device, tgt_device); } else { - btrfs_err_in_rcu(fs_info, + if (scrub_ret != -ECANCELED) + btrfs_err_in_rcu(fs_info, "btrfs_scrub_dev(%s, %llu, %s) failed %d", btrfs_dev_name(src_device), src_device->devid,
As of now only user requested replace cancel can cancel the replace-scrub so no need to log error for it. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/dev-replace.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)