[PATCH 0/9] fix replace-start and replace-cancel racing

[PATCH 0/9] fix replace-start and replace-cancel racing

[Anand Jain]

Replace-start and replace-cancel threads can race to create a messy
situation leading to UAF. We use the scrub code to write
the blocks on the replace target. So if we haven't have set the
replace-scrub-running yet, without this patch we just ignore the error
and free the target device. When this happens the system panics with
UAF error.

Its nice to see that btrfs_dev_replace_finishing() already handles
the ECANCELED (replace canceled) situation, but for an unknown reason
we aren't using it to cleanup the replace cancel situation, instead
we just let the replace cancel ioctl thread to cleanup the target
device and return and out of synchronous with the scrub code.

This patch 4/9, 5/9 and 6/9 uses the return code of btrfs_scrub_cancel()
to check if the scrub was really running. And if its not then shall
return an error to the user (replace not started error) so that user
can retry replace cancel. And uses btrfs_dev_replace_finishing() code
to cleanup after successful cancel of the replace scrub.

Further, a suspended replace, when tries to restart, and if it fails
(for example target device missing, or excl ops running) it goes to the
started state, and so the cli 'btrfs replace status /mnt' hangs with no
progress. So patches 2/9 and 3/9 fixes that.

As the originals code idea of ECANCELED was limited to the situation of
the error only and not user requested, there are unnecessary error log
and warn log which 7/9 and 8/9 patches fixes.

Patches 1/9 and 9/9 are good to have fixes. Makes a function static and
code readability good.

Testing: (I did some attempt to convert these into xfstests but need a
mechanism where kernel thread can wait for user land script. I thought
I could do it using ebfp, but needs more digging on how).
As of now hand tested with using procfs to hold kernel thread at
(wait_for_user(..)) until user land issues go.


1.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
btrfs replace cancel /btrfs
  wait_for_user_go();
btrfs replace status /btrfs

2.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
reboot
mount -o device=/dev/sdc /dev/sdb /btrfs
  wait_for_user_go();
btrfs replace status /btrfs
btrfs replace cancel /btrfs
btrfs replace status /btrfs

3.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
reboot
mount -o degraded /dev/sdb /btrfs
btrfs replace status /btrfs
btrfs replace cancel /btrfs
btrfs replace status /btrfs
umount /btrfs
mount /dev/sdb /btrfs


Anand Jain (9):
  btrfs: mark btrfs_dev_replace_start() as static
  btrfs: replace go back to suspended if target missing
  btrfs: replace back to suspend state if EXCL OP is running
  btrfs: fix UAF due to race between replace start and cancel
  btrfs: replace cancel is successful if scrub cancel is successful
  btrfs: replace's scrub must not be running in replace suspended state
  btrfs: quiten warn if the replace is canceled at finish
  btrfs: user requsted replace cancel is not an error
  btrfs: add explicit check for replace result no error

 fs/btrfs/dev-replace.c | 90 ++++++++++++++++++++++++++++++++++----------------
 fs/btrfs/dev-replace.h |  3 --
 2 files changed, 62 insertions(+), 31 deletions(-)

-- 
1.8.3.1

[PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static

[Anand Jain]

There isn't any other consumer other than in its own file dev-replace.c.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 fs/btrfs/dev-replace.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 2aa48aecc52b..59991165e126 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -390,7 +390,7 @@ static char* btrfs_dev_name(struct btrfs_device *device)
 		return rcu_str_deref(device->name);
 }
 
-int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
+static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
 		int read_src)
 {
diff --git a/fs/btrfs/dev-replace.h b/fs/btrfs/dev-replace.h
index 795c551f5b5e..27e3bb0cca11 100644
--- a/fs/btrfs/dev-replace.h
+++ b/fs/btrfs/dev-replace.h
@@ -13,9 +13,6 @@ int btrfs_run_dev_replace(struct btrfs_trans_handle *trans,
 			  struct btrfs_fs_info *fs_info);
 int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
 			    struct btrfs_ioctl_dev_replace_args *args);
-int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
-		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
-		int read_src);
 void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
 			      struct btrfs_ioctl_dev_replace_args *args);
 int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
-- 
1.8.3.1

Re: [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static

[Nikolay Borisov]

On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> There isn't any other consumer other than in its own file dev-replace.c.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/dev-replace.c | 2 +-
>  fs/btrfs/dev-replace.h | 3 ---
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 2aa48aecc52b..59991165e126 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -390,7 +390,7 @@ static char* btrfs_dev_name(struct btrfs_device *device)
>  		return rcu_str_deref(device->name);
>  }
>  
> -int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> +static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
>  		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
>  		int read_src)
>  {
> diff --git a/fs/btrfs/dev-replace.h b/fs/btrfs/dev-replace.h
> index 795c551f5b5e..27e3bb0cca11 100644
> --- a/fs/btrfs/dev-replace.h
> +++ b/fs/btrfs/dev-replace.h
> @@ -13,9 +13,6 @@ int btrfs_run_dev_replace(struct btrfs_trans_handle *trans,
>  			  struct btrfs_fs_info *fs_info);
>  int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>  			    struct btrfs_ioctl_dev_replace_args *args);
> -int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> -		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
> -		int read_src);
>  void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
>  			      struct btrfs_ioctl_dev_replace_args *args);
>  int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
> 

[PATCH 2/9] btrfs: replace go back to suspended if target missing

[Anand Jain]

At the time of forced unmount we place the running replace to
BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state, so when the system comes
back and suppose the target device is missing, then let the replace
state continue to be in BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state
instead of BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED as there isn't any
matching scrub running as part of replace.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 59991165e126..47d6768a9cde 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -884,6 +884,9 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
 			   "cannot continue dev_replace, tgtdev is missing");
 		btrfs_info(fs_info,
 			   "you may cancel the operation after 'mount -o degraded'");
+		dev_replace->replace_state =
+					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
+		dev_replace->item_needs_writeback = 1;
 		btrfs_dev_replace_write_unlock(dev_replace);
 		return 0;
 	}
-- 
1.8.3.1

Re: [PATCH 2/9] btrfs: replace go back to suspended if target missing

[Nikolay Borisov]

On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> At the time of forced unmount we place the running replace to
> BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state, so when the system comes
> back and suppose the target device is missing, then let the replace
> state continue to be in BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state
> instead of BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED as there isn't any
> matching scrub running as part of replace.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 59991165e126..47d6768a9cde 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -884,6 +884,9 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
>  			   "cannot continue dev_replace, tgtdev is missing");
>  		btrfs_info(fs_info,
>  			   "you may cancel the operation after 'mount -o degraded'");
> +		dev_replace->replace_state =
> +					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
> +		dev_replace->item_needs_writeback = 1;

Why do we need items_needs_writeback = 1 here, nothing is changed w.r.t
on-disk data?

>  		btrfs_dev_replace_write_unlock(dev_replace);
>  		return 0;
>  	}
> 

Re: [PATCH 2/9] btrfs: replace go back to suspended if target missing

[Anand Jain]

On 11/07/2018 08:35 PM, Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> At the time of forced unmount we place the running replace to
>> BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state, so when the system comes
>> back and suppose the target device is missing, then let the replace
>> state continue to be in BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state
>> instead of BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED as there isn't any
>> matching scrub running as part of replace.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/dev-replace.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index 59991165e126..47d6768a9cde 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -884,6 +884,9 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
>>   			   "cannot continue dev_replace, tgtdev is missing");
>>   		btrfs_info(fs_info,
>>   			   "you may cancel the operation after 'mount -o degraded'");
>> +		dev_replace->replace_state =
>> +					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
>> +		dev_replace->item_needs_writeback = 1;
> 
> Why do we need items_needs_writeback = 1 here, nothing is changed w.r.t
> on-disk data?

  You are right. We don't need writeback. Will fix.

Thanks, Anand


>>   		btrfs_dev_replace_write_unlock(dev_replace);
>>   		return 0;
>>   	}
>>

[PATCH 3/9] btrfs: replace back to suspend state if EXCL OP is running

[Anand Jain]

In a secnario where balance and replace co-exists as below,
  start balance; pause balance; start replace; reboot
and when system restarts balance restarts first and the
replace restart will fail as EXCL OP lock is already held by
the balance. If so place the replace state back to
BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 47d6768a9cde..e001c2418940 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -898,6 +898,11 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
 	 * dev-replace to start anyway.
 	 */
 	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+		btrfs_dev_replace_write_lock(dev_replace);
+		dev_replace->replace_state =
+					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
+		dev_replace->item_needs_writeback = 1;
+		btrfs_dev_replace_write_unlock(dev_replace);
 		btrfs_info(fs_info,
 		"cannot resume dev-replace, other exclusive operation running");
 		return 0;
-- 
1.8.3.1

[PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel

[Anand Jain]

replace cancel thread can race with the replace start thread and if
fs_info::scrubs_running is not yet set the btrfs_scrub_cancel() will fail
to stop the scrub thread, so the scrub thread continue with the scrub for
replace which then shall try to write to the target device and which is
already freed by the cancel thread. Please ref to the logs below.

So scrub_setup_ctx() warns as tgtdev is null.

static noinline_for_stack
struct scrub_ctx *scrub_setup_ctx(struct btrfs_device *dev, int
is_dev_replace)
{
::
        if (is_dev_replace) {
                WARN_ON(!fs_info->dev_replace.tgtdev);  <===
                sctx->pages_per_wr_bio = SCRUB_PAGES_PER_WR_BIO;
                sctx->wr_tgtdev = fs_info->dev_replace.tgtdev;
                sctx->flush_all_writes = false;
        }

[ 6724.497655] BTRFS info (device sdb): dev_replace from /dev/sdb (devid 1) to /dev/sdc started
[ 6753.945017] BTRFS info (device sdb): dev_replace from /dev/sdb (devid 1) to /dev/sdc canceled
[ 6852.426700] WARNING: CPU: 0 PID: 4494 at fs/btrfs/scrub.c:622 scrub_setup_ctx.isra.19+0x220/0x230 [btrfs]
::
[ 6852.428928] RIP: 0010:scrub_setup_ctx.isra.19+0x220/0x230 [btrfs]
::
[ 6852.432970] Call Trace:
[ 6852.433202]  btrfs_scrub_dev+0x19b/0x5c0 [btrfs]
[ 6852.433471]  btrfs_dev_replace_start+0x48c/0x6a0 [btrfs]
[ 6852.433800]  btrfs_dev_replace_by_ioctl+0x3a/0x60 [btrfs]
[ 6852.434097]  btrfs_ioctl+0x2476/0x2d20 [btrfs]
[ 6852.434365]  ? do_sigaction+0x7d/0x1e0
[ 6852.434623]  do_vfs_ioctl+0xa9/0x6c0
[ 6852.434865]  ? syscall_trace_enter+0x1c8/0x310
[ 6852.435124]  ? syscall_trace_enter+0x1c8/0x310
[ 6852.435387]  ksys_ioctl+0x60/0x90
[ 6852.435663]  __x64_sys_ioctl+0x16/0x20
[ 6852.435907]  do_syscall_64+0x50/0x180
[ 6852.436150]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Further, as the replace thread enters the
scrub_write_page_to_dev_replace() without the target device it panics

static int scrub_add_page_to_wr_bio(struct scrub_ctx *sctx,
                                    struct scrub_page *spage)
{
::
      bio_set_dev(bio, sbio->dev->bdev); <======

[ 6929.715145] BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0
::
[ 6929.717106] Workqueue: btrfs-scrub btrfs_scrub_helper [btrfs]
[ 6929.717420] RIP: 0010:scrub_write_page_to_dev_replace+0xb4/0x260
[btrfs]
::
[ 6929.721430] Call Trace:
[ 6929.721663]  scrub_write_block_to_dev_replace+0x3f/0x60 [btrfs]
[ 6929.721975]  scrub_bio_end_io_worker+0x1af/0x490 [btrfs]
[ 6929.722277]  normal_work_helper+0xf0/0x4c0 [btrfs]
[ 6929.722552]  process_one_work+0x1f4/0x520
[ 6929.722805]  ? process_one_work+0x16e/0x520
[ 6929.723063]  worker_thread+0x46/0x3d0
[ 6929.723313]  kthread+0xf8/0x130
[ 6929.723544]  ? process_one_work+0x520/0x520
[ 6929.723800]  ? kthread_delayed_work_timer_fn+0x80/0x80
[ 6929.724081]  ret_from_fork+0x3a/0x50

Fix this by letting the btrfs_dev_replace_finishing() to do the job of
cleaning after the cancel, including freeing of the target device.
btrfs_dev_replace_finishing() is called when btrfs_scub_dev() returns
along with the scrub return status.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 61 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index e001c2418940..90c124edec76 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -797,39 +797,56 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
 		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
 		btrfs_dev_replace_write_unlock(dev_replace);
-		goto leave;
+		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
+		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
+		tgt_device = dev_replace->tgtdev;
+		src_device = dev_replace->srcdev;
+		btrfs_dev_replace_write_unlock(dev_replace);
+		btrfs_scrub_cancel(fs_info);
+		/*
+		 * btrfs_dev_replace_finishing() will handle the cleanup part
+		 */
+		btrfs_info_in_rcu(fs_info,
+			"dev_replace from %s (devid %llu) to %s canceled",
+			btrfs_dev_name(src_device), src_device->devid,
+			btrfs_dev_name(tgt_device));
+		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
+		/*
+		 * scrub doing the replace isn't running so do the cleanup here
+		 */
 		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
 		tgt_device = dev_replace->tgtdev;
 		src_device = dev_replace->srcdev;
 		dev_replace->tgtdev = NULL;
 		dev_replace->srcdev = NULL;
-		break;
-	}
-	dev_replace->replace_state = BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED;
-	dev_replace->time_stopped = ktime_get_real_seconds();
-	dev_replace->item_needs_writeback = 1;
-	btrfs_dev_replace_write_unlock(dev_replace);
-	btrfs_scrub_cancel(fs_info);
+		dev_replace->replace_state = BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED;
+		dev_replace->time_stopped = ktime_get_real_seconds();
+		dev_replace->item_needs_writeback = 1;
 
-	trans = btrfs_start_transaction(root, 0);
-	if (IS_ERR(trans)) {
-		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
-		return PTR_ERR(trans);
-	}
-	ret = btrfs_commit_transaction(trans);
-	WARN_ON(ret);
+		btrfs_dev_replace_write_unlock(dev_replace);
 
-	btrfs_info_in_rcu(fs_info,
-		"dev_replace from %s (devid %llu) to %s canceled",
-		btrfs_dev_name(src_device), src_device->devid,
-		btrfs_dev_name(tgt_device));
+		btrfs_scrub_cancel(fs_info);
+
+		trans = btrfs_start_transaction(root, 0);
+		if (IS_ERR(trans)) {
+			mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
+			return PTR_ERR(trans);
+		}
+		ret = btrfs_commit_transaction(trans);
+		WARN_ON(ret);
 
-	if (tgt_device)
-		btrfs_destroy_dev_replace_tgtdev(tgt_device);
+		btrfs_info_in_rcu(fs_info,
+		"suspended dev_replace from %s (devid %llu) to %s canceled",
+			btrfs_dev_name(src_device), src_device->devid,
+			btrfs_dev_name(tgt_device));
+
+		if (tgt_device)
+			btrfs_destroy_dev_replace_tgtdev(tgt_device);
+		break;
+	}
 
-leave:
 	mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
 	return result;
 }
-- 
1.8.3.1

[PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful

[Anand Jain]

In btrfs_dev_replace_cancel() we should check if the
btrfs_scrub_cancel() is successful. If the btrfs_scrub_cancel() fails, return
BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED so that user can try to
cancel the replace again.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 90c124edec76..c092ed559714 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -799,18 +799,22 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 		btrfs_dev_replace_write_unlock(dev_replace);
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
-		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
 		tgt_device = dev_replace->tgtdev;
 		src_device = dev_replace->srcdev;
 		btrfs_dev_replace_write_unlock(dev_replace);
-		btrfs_scrub_cancel(fs_info);
-		/*
-		 * btrfs_dev_replace_finishing() will handle the cleanup part
-		 */
-		btrfs_info_in_rcu(fs_info,
-			"dev_replace from %s (devid %llu) to %s canceled",
-			btrfs_dev_name(src_device), src_device->devid,
-			btrfs_dev_name(tgt_device));
+		ret = btrfs_scrub_cancel(fs_info);
+		if (ret) {
+			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
+		} else {
+			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
+			/*
+			 * btrfs_dev_replace_finishing() will handle the cleanup part
+			 */
+			btrfs_info_in_rcu(fs_info,
+				"dev_replace from %s (devid %llu) to %s canceled",
+				btrfs_dev_name(src_device), src_device->devid,
+				btrfs_dev_name(tgt_device));
+		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
 		/*
-- 
1.8.3.1

Re: [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful

[Nikolay Borisov]

On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> In btrfs_dev_replace_cancel() we should check if the
> btrfs_scrub_cancel() is successful. If the btrfs_scrub_cancel() fails, return
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED so that user can try to
> cancel the replace again.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 90c124edec76..c092ed559714 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -799,18 +799,22 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
>  		btrfs_dev_replace_write_unlock(dev_replace);
>  		break;
>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
> -		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
>  		tgt_device = dev_replace->tgtdev;
>  		src_device = dev_replace->srcdev;
>  		btrfs_dev_replace_write_unlock(dev_replace);
> -		btrfs_scrub_cancel(fs_info);
> -		/*
> -		 * btrfs_dev_replace_finishing() will handle the cleanup part
> -		 */
> -		btrfs_info_in_rcu(fs_info,
> -			"dev_replace from %s (devid %llu) to %s canceled",
> -			btrfs_dev_name(src_device), src_device->devid,
> -			btrfs_dev_name(tgt_device));
> +		ret = btrfs_scrub_cancel(fs_info);
> +		if (ret) {
> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
> +		} else {
> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
> +			/*
> +			 * btrfs_dev_replace_finishing() will handle the cleanup part
> +			 */
> +			btrfs_info_in_rcu(fs_info,
> +				"dev_replace from %s (devid %llu) to %s canceled",
> +				btrfs_dev_name(src_device), src_device->devid,
> +				btrfs_dev_name(tgt_device));

This is identical to the btrfs_info_in_rcu several lines further down.
So if btrfs_scrub_cancel is successful you will print this messages
twice. Furthermore, there is already an unconditinal call to
btrfs_scrub_cancel. You are duplicating this function, this definitely
needs another look...

> +		}
>  		break;
>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
>  		/*
> 

Re: [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful

[Nikolay Borisov]

On 7.11.18 г. 14:25 ч., Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> In btrfs_dev_replace_cancel() we should check if the
>> btrfs_scrub_cancel() is successful. If the btrfs_scrub_cancel() fails, return
>> BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED so that user can try to
>> cancel the replace again.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>  fs/btrfs/dev-replace.c | 22 +++++++++++++---------
>>  1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index 90c124edec76..c092ed559714 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -799,18 +799,22 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
>>  		btrfs_dev_replace_write_unlock(dev_replace);
>>  		break;
>>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
>> -		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
>>  		tgt_device = dev_replace->tgtdev;
>>  		src_device = dev_replace->srcdev;
>>  		btrfs_dev_replace_write_unlock(dev_replace);
>> -		btrfs_scrub_cancel(fs_info);
>> -		/*
>> -		 * btrfs_dev_replace_finishing() will handle the cleanup part
>> -		 */
>> -		btrfs_info_in_rcu(fs_info,
>> -			"dev_replace from %s (devid %llu) to %s canceled",
>> -			btrfs_dev_name(src_device), src_device->devid,
>> -			btrfs_dev_name(tgt_device));
>> +		ret = btrfs_scrub_cancel(fs_info);
>> +		if (ret) {
>> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
>> +		} else {
>> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
>> +			/*
>> +			 * btrfs_dev_replace_finishing() will handle the cleanup part
>> +			 */
>> +			btrfs_info_in_rcu(fs_info,
>> +				"dev_replace from %s (devid %llu) to %s canceled",
>> +				btrfs_dev_name(src_device), src_device->devid,
>> +				btrfs_dev_name(tgt_device));
> 
> This is identical to the btrfs_info_in_rcu several lines further down.
> So if btrfs_scrub_cancel is successful you will print this messages
> twice. Furthermore, there is already an unconditinal call to
> btrfs_scrub_cancel. You are duplicating this function, this definitely
> needs another look...

Ah, it builds on top of the previous patch which I still haven't
reviewed. So ignore this, my bad.
> 
>> +		}
>>  		break;
>>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
>>  		/*
>>

[PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state

[Anand Jain]

When the replace state is placed in the suspended state,
btrfs_scrub_cancel() should fail with -ENOTCONN as there is no
scrub running, as a safety catch check if btrfs_scrub_cancel()
returns -ENOTCONN and assert if it doesn't.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index c092ed559714..dae2b920f1a9 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -831,7 +831,9 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 
 		btrfs_dev_replace_write_unlock(dev_replace);
 
-		btrfs_scrub_cancel(fs_info);
+		/* scrub for replace must not be running in suspended state */
+		if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
+			ASSERT(0);
 
 		trans = btrfs_start_transaction(root, 0);
 		if (IS_ERR(trans)) {
-- 
1.8.3.1

Re: [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state

[Nikolay Borisov]

On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> +		/* scrub for replace must not be running in suspended state */
> +		if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
> +			ASSERT(0);

ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)

Re: [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state

[Anand Jain]

On 11/07/2018 08:19 PM, Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> +		/* scrub for replace must not be running in suspended state */
>> +		if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
>> +			ASSERT(0);
> 
> ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)
> 

There will be substantial difference in code when compiled with and 
without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info)
won't be run at all,  I would like to keep it as it is.

[1]
------
./fs/btrfs/ctree.h
#ifdef CONFIG_BTRFS_ASSERT

__cold
static inline void assfail(const char *expr, const char *file, int line)
{
         pr_err("assertion failed: %s, file: %s, line: %d\n",
                expr, file, line);
         BUG();
}

#define ASSERT(expr)    \
         (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
#else
#define ASSERT(expr)    ((void)0)
#endif
-------

Thanks, Anand

Re: [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state

[Nikolay Borisov]

On 8.11.18 г. 10:33 ч., Anand Jain wrote:
> 
> 
> On 11/07/2018 08:19 PM, Nikolay Borisov wrote:
>>
>>
>> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>>> +        /* scrub for replace must not be running in suspended state */
>>> +        if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
>>> +            ASSERT(0);
>>
>> ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)
>>
> 
> There will be substantial difference in code when compiled with and
> without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info)
> won't be run at all,  I would like to keep it as it is.

Fair point, in that case do:

ret = btrfs_scrub_cancel(fs_info);
ASSERT(ret != -ENOTCONN);

result


> 
> [1]
> ------
> ./fs/btrfs/ctree.h
> #ifdef CONFIG_BTRFS_ASSERT
> 
> __cold
> static inline void assfail(const char *expr, const char *file, int line)
> {
>         pr_err("assertion failed: %s, file: %s, line: %d\n",
>                expr, file, line);
>         BUG();
> }
> 
> #define ASSERT(expr)    \
>         (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> #else
> #define ASSERT(expr)    ((void)0)
> #endif
> -------
> 
> Thanks, Anand
> 
> 

Re: [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state

[Anand Jain]

On 11/08/2018 04:52 PM, Nikolay Borisov wrote:
> 
> 
> On 8.11.18 г. 10:33 ч., Anand Jain wrote:
>>
>>
>> On 11/07/2018 08:19 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>>>> +        /* scrub for replace must not be running in suspended state */
>>>> +        if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
>>>> +            ASSERT(0);
>>>
>>> ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)
>>>
>>
>> There will be substantial difference in code when compiled with and
>> without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info)
>> won't be run at all,  I would like to keep it as it is.
> 
> Fair point, in that case do:
> 
> ret = btrfs_scrub_cancel(fs_info);
> ASSERT(ret != -ENOTCONN);

Fixed.

Thanks, Anand

> result
> 
> 
>>
>> [1]
>> ------
>> ./fs/btrfs/ctree.h
>> #ifdef CONFIG_BTRFS_ASSERT
>>
>> __cold
>> static inline void assfail(const char *expr, const char *file, int line)
>> {
>>          pr_err("assertion failed: %s, file: %s, line: %d\n",
>>                 expr, file, line);
>>          BUG();
>> }
>>
>> #define ASSERT(expr)    \
>>          (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
>> #else
>> #define ASSERT(expr)    ((void)0)
>> #endif
>> -------
>>
>> Thanks, Anand
>>
>>

[PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish

[Anand Jain]

When we successfully cancel the replace its scrub returns -ECANCELED,
which then passed to btrfs_dev_replace_finishing(), it cleans up based
on the scrub returned status and propagates the same -ECANCELED back
the parent function. So skip the -ECANCELED error to log the WARN.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index dae2b920f1a9..c14c41b70287 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -497,7 +497,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 	ret = btrfs_dev_replace_finishing(fs_info, ret);
 	if (ret == -EINPROGRESS) {
 		ret = BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS;
-	} else {
+	} else if (ret != -ECANCELED) {
 		WARN_ON(ret);
 	}
 
@@ -956,7 +956,8 @@ static int btrfs_dev_replace_kthread(void *data)
 			      btrfs_device_get_total_bytes(dev_replace->srcdev),
 			      &dev_replace->scrub_progress, 0, 1);
 	ret = btrfs_dev_replace_finishing(fs_info, ret);
-	WARN_ON(ret);
+	if (ret != -ECANCELED)
+		WARN_ON(ret);
 
 	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
 	return 0;
-- 
1.8.3.1

Re: [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish

[Nikolay Borisov]

On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> -	WARN_ON(ret);
> +	if (ret != -ECANCELED)
> +		WARN_ON(ret);

WARN_ON(ret && ret != -ECANCELED)

Re: [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish

[Anand Jain]

On 11/07/2018 08:17 PM, Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> -	WARN_ON(ret);
>> +	if (ret != -ECANCELED)
>> +		WARN_ON(ret);
> 
> WARN_ON(ret && ret != -ECANCELED)
> 

Will fix.
Thanks, Anand

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

[Anand Jain]

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(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index c14c41b70287..cf3554554616 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,
-- 
1.8.3.1

[PATCH 9/9] btrfs: add explicit check for replace result no error

[Anand Jain]

We recast the replace return status
BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS to 0, to indicate no
error.
And since BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR should also return 0,
which is also declared as 0, so we just return. Instead add it to the if
statement so that there is enough clarity while reading the code.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index cf3554554616..ca44998189c7 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -533,8 +533,9 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
 					args->start.cont_reading_from_srcdev_mode);
 	args->result = ret;
 	/* don't warn if EINPROGRESS, someone else might be running scrub */
-	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS)
-		ret = 0;
+	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS ||
+	    ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR)
+		return 0;
 
 	return ret;
 }
-- 
1.8.3.1

Re: [PATCH 9/9] btrfs: add explicit check for replace result no error

[Nikolay Borisov]

On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> We recast the replace return status
> BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS to 0, to indicate no
> error.
> And since BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR should also return 0,
> which is also declared as 0, so we just return. Instead add it to the if
> statement so that there is enough clarity while reading the code.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index cf3554554616..ca44998189c7 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -533,8 +533,9 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>  					args->start.cont_reading_from_srcdev_mode);
>  	args->result = ret;
>  	/* don't warn if EINPROGRESS, someone else might be running scrub */
> -	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS)
> -		ret = 0;
> +	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS ||
> +	    ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR)

BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR can only be returned from
btrfs_dev_replace_cancel, so what you are doing with checking ret ==
BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR is redundant. So using
BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR is at the very least misleading
here.

I suggest you drop this patch.

> +		return 0;
>  
>  	return ret;
>  }
> 

Re: [PATCH 9/9] btrfs: add explicit check for replace result no error

[Anand Jain]

On 11/07/2018 08:15 PM, Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> We recast the replace return status
>> BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS to 0, to indicate no
>> error.
>> And since BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR should also return 0,
>> which is also declared as 0, so we just return. Instead add it to the if
>> statement so that there is enough clarity while reading the code.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/dev-replace.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index cf3554554616..ca44998189c7 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -533,8 +533,9 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>>   					args->start.cont_reading_from_srcdev_mode);
>>   	args->result = ret;
>>   	/* don't warn if EINPROGRESS, someone else might be running scrub */
>> -	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS)
>> -		ret = 0;
>> +	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS ||
>> +	    ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR)
> 
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR can only be returned from
> btrfs_dev_replace_cancel,

  It can return 0 and which is also
  BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR which this patch makes it
  explicit.

  Looking at this again tells me that, as btrfs_dev_replace_start()
  is internal helper function, its better if it free from all usage
  of BTRFS_IOCTL_DEV_REPLACE_RESULT*.

Thanks, Anand

> so what you are doing with checking ret ==
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR is redundant. So using
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR is at the very least misleading
> here.

> I suggest you drop this patch.
> 
>> +		return 0;
>>   
>>   	return ret;
>>   }
>>

[PATCH 0/9 v2] fix replace-start and replace-cancel racing

[Anand Jain]

v1->v2:
  2/9: Drop writeback required
  3/9: Drop writeback required
  7/9: Use the condition within the WARN_ON()
  6/9: Use the condition within the ASSERT()

Replace-start and replace-cancel threads can race to create a messy
situation leading to UAF. We use the scrub code to write
the blocks on the replace target. So if we haven't have set the
replace-scrub-running yet, without this patch we just ignore the error
and free the target device. When this happens the system panics with
UAF error.

Its nice to see that btrfs_dev_replace_finishing() already handles
the ECANCELED (replace canceled) situation, but for an unknown reason
we aren't using it to cleanup the replace cancel situation, instead
we just let the replace cancel ioctl thread to cleanup the target
device and return and out of synchronous with the scrub code.

This patch 4/9, 5/9 and 6/9 uses the return code of btrfs_scrub_cancel()
to check if the scrub was really running. And if its not then shall
return an error to the user (replace not started error) so that user
can retry replace cancel. And uses btrfs_dev_replace_finishing() code
to cleanup after successful cancel of the replace scrub.

Further, a suspended replace, when tries to restart, and if it fails
(for example target device missing, or excl ops running) it goes to the
started state, and so the cli 'btrfs replace status /mnt' hangs with no
progress. So patches 2/9 and 3/9 fixes that.

As the originals code idea of ECANCELED was limited to the situation of
the error only and not user requested, there are unnecessary error log
and warn log which 7/9 and 8/9 patches fixes.

Patches 1/9 and 9/9 are good to have fixes. Makes a function static and
code readability good.

Testing: (I did some attempt to convert these into xfstests but need a
mechanism where kernel thread can wait for user land script. I thought
I could do it using ebfp, but needs more digging on how).
As of now hand tested with using procfs to hold kernel thread at
(wait_for_user(..)) until user land issues go.


1.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
btrfs replace cancel /btrfs
  wait_for_user_go();
btrfs replace status /btrfs

2.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
reboot
mount -o device=/dev/sdc /dev/sdb /btrfs
  wait_for_user_go();
btrfs replace status /btrfs
btrfs replace cancel /btrfs
btrfs replace status /btrfs

3.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
reboot
mount -o degraded /dev/sdb /btrfs
btrfs replace status /btrfs
btrfs replace cancel /btrfs
btrfs replace status /btrfs
umount /btrfs
mount /dev/sdb /btrfs

Anand Jain (9):
  btrfs: mark btrfs_dev_replace_start() as static
  btrfs: replace go back to suspended if target missing
  btrfs: replace back to suspend state if EXCL OP is running
  btrfs: fix UAF due to race between replace start and cancel
  btrfs: replace cancel is successful if scrub cancel is successful
  btrfs: replace's scrub must not be running in replace suspended state
  btrfs: quiten warn if the replace is canceled at finish
  btrfs: user requsted replace cancel is not an error
  btrfs: add explicit check for replace result no error

 fs/btrfs/dev-replace.c | 87 ++++++++++++++++++++++++++++++++++----------------
 fs/btrfs/dev-replace.h |  3 --
 2 files changed, 59 insertions(+), 31 deletions(-)

-- 
1.8.3.1

From e5a84ccf4f73ec405bc7f5ad812b04762f287e2d Mon Sep 17 00:00:00 2001
From: Anand Jain <anand.jain@oracle.com>
Date: Wed, 7 Nov 2018 18:51:19 +0800


Anand Jain (9):
  btrfs: mark btrfs_dev_replace_start() as static
  btrfs: replace go back to suspended if target missing
  btrfs: replace back to suspend state if EXCL OP is running
  btrfs: fix UAF due to race between replace start and cancel
  btrfs: replace cancel is successful if scrub cancel is successful
  btrfs: replace's scrub must not be running in replace suspended state
  btrfs: quiten warn if the replace is canceled at finish
  btrfs: user requsted replace cancel is not an error
  btrfs: add explicit check for replace result no error

 fs/btrfs/dev-replace.c | 90 ++++++++++++++++++++++++++++++++++----------------
 fs/btrfs/dev-replace.h |  3 --
 2 files changed, 62 insertions(+), 31 deletions(-)

-- 
1.8.3.1

[PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static

[Anand Jain]

There isn't any other consumer other than in its own file dev-replace.c.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 fs/btrfs/dev-replace.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 2aa48aecc52b..59991165e126 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -390,7 +390,7 @@ static char* btrfs_dev_name(struct btrfs_device *device)
 		return rcu_str_deref(device->name);
 }
 
-int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
+static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
 		int read_src)
 {
diff --git a/fs/btrfs/dev-replace.h b/fs/btrfs/dev-replace.h
index 795c551f5b5e..27e3bb0cca11 100644
--- a/fs/btrfs/dev-replace.h
+++ b/fs/btrfs/dev-replace.h
@@ -13,9 +13,6 @@ int btrfs_run_dev_replace(struct btrfs_trans_handle *trans,
 			  struct btrfs_fs_info *fs_info);
 int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
 			    struct btrfs_ioctl_dev_replace_args *args);
-int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
-		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
-		int read_src);
 void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
 			      struct btrfs_ioctl_dev_replace_args *args);
 int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
-- 
1.8.3.1