From: David Sterba <dsterba@suse.cz>
To: Anand Jain <anand.jain@oracle.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel
Date: Tue, 13 Nov 2018 18:24:48 +0100 [thread overview]
Message-ID: <20181113172448.GF24115@twin.jikos.cz> (raw)
In-Reply-To: <1541946144-8174-5-git-send-email-anand.jain@oracle.com>
On Sun, Nov 11, 2018 at 10:22:19PM +0800, Anand Jain wrote:
> 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 35ce10f18607..d32d696d931c 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;
There's a compiler warning:
fs/btrfs/dev-replace.c: In function ‘btrfs_dev_replace_cancel’:
fs/btrfs/dev-replace.c:865:9: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized]
return result;
^~~~~~
I haven't looked closer though it looks valid.
next prev parent reply other threads:[~2018-11-13 17:25 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-11 14:22 [PATCH 0/9 v2] fix replace-start and replace-cancel racing Anand Jain
2018-11-11 14:22 ` [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static Anand Jain
2018-11-11 14:22 ` [PATCH 2/9] btrfs: replace go back to suspended if target missing Anand Jain
2018-11-11 14:22 ` [PATCH 3/9] btrfs: replace back to suspend state if EXCL OP is running Anand Jain
2018-11-11 14:22 ` [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel Anand Jain
2018-11-13 17:24 ` David Sterba [this message]
2018-11-14 1:28 ` Anand Jain
2018-11-15 14:00 ` David Sterba
2018-11-15 15:25 ` David Sterba
2018-11-16 6:37 ` Anand Jain
2018-11-14 5:50 ` [PATCH 4/9 v2.1] " Anand Jain
2018-11-11 14:22 ` [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful Anand Jain
2018-11-15 15:27 ` David Sterba
2018-11-16 6:38 ` Anand Jain
2018-11-11 14:22 ` [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state Anand Jain
2018-11-11 14:22 ` [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish Anand Jain
2018-11-15 15:35 ` David Sterba
2018-11-16 12:06 ` Anand Jain
2018-11-16 18:49 ` David Sterba
2018-11-11 14:22 ` [PATCH 8/9] btrfs: user requsted replace cancel is not an error Anand Jain
2018-11-15 15:31 ` David Sterba
2018-11-16 10:29 ` Anand Jain
2018-11-16 11:05 ` Anand Jain
2018-11-11 14:22 ` [PATCH 9/9] btrfs: add explicit check for replace result no error Anand Jain
2018-11-13 17:33 ` [PATCH 0/9 v2] fix replace-start and replace-cancel racing David Sterba
2018-11-15 15:41 ` David Sterba
2018-11-16 9:32 ` Anand Jain
-- strict thread matches above, loose matches on Subject: below --
2018-11-07 11:43 [PATCH 0/9] " Anand Jain
2018-11-07 11:43 ` [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel Anand Jain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181113172448.GF24115@twin.jikos.cz \
--to=dsterba@suse.cz \
--cc=anand.jain@oracle.com \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).