From: Anand Jain <anand.jain@oracle.com>
To: dsterba@suse.cz, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel
Date: Wed, 14 Nov 2018 09:28:34 +0800 [thread overview]
Message-ID: <e1e32e53-3860-1d74-c2ed-1b4dbdb57404@oracle.com> (raw)
In-Reply-To: <20181113172448.GF24115@twin.jikos.cz>
On 11/14/2018 01:24 AM, David Sterba wrote:
> 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.
>
int result; is assigned within switch(), so there isn't actual problem.
But will initialize the result to -EINVAL to quite the compiler.
Sending v3.
Thanks, Anand
next prev parent reply other threads:[~2018-11-14 1:28 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
2018-11-14 1:28 ` Anand Jain [this message]
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=e1e32e53-3860-1d74-c2ed-1b4dbdb57404@oracle.com \
--to=anand.jain@oracle.com \
--cc=dsterba@suse.cz \
--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).