Re: [PATCH 8/9] btrfs: user requsted replace cancel is not an error

[Anand Jain <anand.jain@oracle.com>]

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