| Message ID | 20170203082023.3577-6-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 03, 2017 at 04:20:23PM +0800, Qu Wenruo wrote: > When dev-replace and scrub are run at the same time, dev-replace can be > canceled by scrub. It's quite common for btrfs/069. > > The backtrace would be like: > general protection fault: 0000 [#1] SMP > Workqueue: btrfs-endio-raid56 btrfs_endio_raid56_helper [btrfs] > RIP: 0010:[<ffffffff813a2fa8>] [<ffffffff813a2fa8>] generic_make_request_checks+0x198/0x5a0 > Call Trace: > [<ffffffff813a4cff>] ? generic_make_request+0xcf/0x290 > [<ffffffff813a4c54>] generic_make_request+0x24/0x290 > [<ffffffff813a4cff>] ? generic_make_request+0xcf/0x290 > [<ffffffff813a4f2e>] submit_bio+0x6e/0x120 > [<ffffffffa087279d>] ? page_in_rbio+0x4d/0x80 [btrfs] > [<ffffffffa08737d0>] ? rbio_orig_end_io+0x80/0x80 [btrfs] > [<ffffffffa0873e31>] finish_rmw+0x401/0x550 [btrfs] > [<ffffffffa0874fc6>] validate_rbio_for_rmw+0x36/0x40 [btrfs] > [<ffffffffa087504d>] raid_rmw_end_io+0x7d/0x90 [btrfs] > [<ffffffff8139c536>] bio_endio+0x56/0x60 > [<ffffffffa07e6e5c>] end_workqueue_fn+0x3c/0x40 [btrfs] > [<ffffffffa08285bf>] btrfs_scrubparity_helper+0xef/0x610 [btrfs] > [<ffffffffa0828b9e>] btrfs_endio_raid56_helper+0xe/0x10 [btrfs] > [<ffffffff810ec8df>] process_one_work+0x2af/0x720 > [<ffffffff810ec85b>] ? process_one_work+0x22b/0x720 > [<ffffffff810ecd9b>] worker_thread+0x4b/0x4f0 > [<ffffffff810ecd50>] ? process_one_work+0x720/0x720 > [<ffffffff810ecd50>] ? process_one_work+0x720/0x720 > [<ffffffff810f39d3>] kthread+0xf3/0x110 > [<ffffffff810f38e0>] ? kthread_park+0x60/0x60 > [<ffffffff81857647>] ret_from_fork+0x27/0x40 > > While in that case, target device can be destroyed at cancel time, > leading to a user-after-free bug: > > Process A (dev-replace) | Process B(scrub) > ---------------------------------------------------------------------- > |(Any RW is OK) > |scrub_setup_recheck_block() > ||- btrfs_map_sblock() > | Got a bbio with tgtdev > btrfs_dev_replace_finishing() | > |- btrfs_destory_dev_replace_tgtdev()| > |- call_rcu(free_device) | > |- __free_device() | > |- kfree(device) | > | Scrub worker: > | Access bbio->stripes[], which > | contains tgtdev. > | This triggers general protection. > We already have bio_counter to block device replace to avoid use-after-free problem, is there any particular reason of not using it? Thanks, -liubo > The bug is mostly obvious for RAID5/6 since raid56 choose to keep old > rbio and rbio->bbio for later steal, this hugely enlarged the race > window and makes it much easier to trigger the bug. > > This patch introduces 'tgtdev_refs' and 'tgtdev_wait' for btrfs_device > to wait for all its user released the target device. > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > fs/btrfs/dev-replace.c | 7 ++++++- > fs/btrfs/volumes.c | 36 +++++++++++++++++++++++++++++++++++- > fs/btrfs/volumes.h | 10 ++++++++++ > 3 files changed, 51 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c > index 5de280b9ad73..794a6a0bedf2 100644 > --- a/fs/btrfs/dev-replace.c > +++ b/fs/btrfs/dev-replace.c > @@ -558,7 +558,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, > rcu_str_deref(src_device->name), > src_device->devid, > rcu_str_deref(tgt_device->name)); > - tgt_device->is_tgtdev_for_dev_replace = 0; > tgt_device->devid = src_device->devid; > src_device->devid = BTRFS_DEV_REPLACE_DEVID; > memcpy(uuid_tmp, tgt_device->uuid, sizeof(uuid_tmp)); > @@ -579,6 +578,12 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, > > btrfs_dev_replace_unlock(dev_replace, 1); > > + /* > + * Only change is_tgtdev_for_dev_replace flag after all its > + * users get released. > + */ > + wait_target_device(tgt_device); > + tgt_device->is_tgtdev_for_dev_replace = 0; > btrfs_rm_dev_replace_blocked(fs_info); > > btrfs_rm_dev_replace_remove_srcdev(fs_info, src_device); > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 3c3c69c0eee4..84db9fb22b7d 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2064,6 +2064,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, > WARN_ON(!tgtdev); > mutex_lock(&fs_info->fs_devices->device_list_mutex); > > + wait_target_device(tgtdev); > btrfs_sysfs_rm_device_link(fs_info->fs_devices, tgtdev); > > if (tgtdev->bdev) > @@ -2598,6 +2599,8 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, > device->is_tgtdev_for_dev_replace = 1; > device->mode = FMODE_EXCL; > device->dev_stats_valid = 1; > + atomic_set(&device->tgtdev_refs, 0); > + init_waitqueue_head(&device->tgtdev_wait); > set_blocksize(device->bdev, 4096); > device->fs_devices = fs_info->fs_devices; > list_add(&device->dev_list, &fs_info->fs_devices->devices); > @@ -2624,6 +2627,8 @@ void btrfs_init_dev_replace_tgtdev_for_resume(struct btrfs_fs_info *fs_info, > tgtdev->sector_size = sectorsize; > tgtdev->fs_info = fs_info; > tgtdev->in_fs_metadata = 1; > + atomic_set(&tgtdev->tgtdev_refs, 0); > + init_waitqueue_head(&tgtdev->tgtdev_wait); > } > > static noinline int btrfs_update_device(struct btrfs_trans_handle *trans, > @@ -5302,6 +5307,32 @@ static struct btrfs_bio *alloc_btrfs_bio(int total_stripes, int real_stripes) > return bbio; > } > > +static void pin_bbio_target_device(struct btrfs_bio *bbio) > +{ > + int i; > + > + for (i = 0; i < bbio->num_stripes; i++) { > + struct btrfs_device *device = bbio->stripes[i].dev; > + > + if (device->is_tgtdev_for_dev_replace) > + atomic_inc(&device->tgtdev_refs); > + } > +} > + > +static void unpin_bbio_target_device(struct btrfs_bio *bbio) > +{ > + int i; > + > + for (i = 0; i < bbio->num_stripes; i++) { > + struct btrfs_device *device = bbio->stripes[i].dev; > + > + if (device->is_tgtdev_for_dev_replace) { > + atomic_dec(&device->tgtdev_refs); > + wake_up(&device->tgtdev_wait); > + } > + } > +} > + > void btrfs_get_bbio(struct btrfs_bio *bbio) > { > WARN_ON(!atomic_read(&bbio->refs)); > @@ -5312,8 +5343,10 @@ void btrfs_put_bbio(struct btrfs_bio *bbio) > { > if (!bbio) > return; > - if (atomic_dec_and_test(&bbio->refs)) > + if (atomic_dec_and_test(&bbio->refs)) { > + unpin_bbio_target_device(bbio); > kfree(bbio); > + } > } > > static int __btrfs_map_block(struct btrfs_fs_info *fs_info, > @@ -5868,6 +5901,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info, > bbio->stripes[0].physical = physical_to_patch_in_first_stripe; > bbio->mirror_num = map->num_stripes + 1; > } > + pin_bbio_target_device(bbio); > out: > if (dev_replace_is_ongoing) { > btrfs_dev_replace_clear_lock_blocking(dev_replace); > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index 24ba6bc3ec34..702361182597 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -149,6 +149,10 @@ struct btrfs_device { > /* Counter to record the change of device stats */ > atomic_t dev_stats_ccnt; > atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX]; > + > + /* To ensure we wait this target device before destroying it */ > + atomic_t tgtdev_refs; > + wait_queue_head_t tgtdev_wait; > }; > > /* > @@ -538,4 +542,10 @@ struct list_head *btrfs_get_fs_uuids(void); > void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info); > void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info); > > +static inline void wait_target_device(struct btrfs_device *tgtdev) > +{ > + if (!tgtdev || !tgtdev->is_tgtdev_for_dev_replace) > + return; > + wait_event(tgtdev->tgtdev_wait, atomic_read(&tgtdev->tgtdev_refs) == 0); > +} > #endif > -- > 2.11.0 > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 03/18/2017 10:12 AM, Liu Bo wrote: > On Fri, Feb 03, 2017 at 04:20:23PM +0800, Qu Wenruo wrote: >> When dev-replace and scrub are run at the same time, dev-replace can be >> canceled by scrub. It's quite common for btrfs/069. >> >> The backtrace would be like: >> general protection fault: 0000 [#1] SMP >> Workqueue: btrfs-endio-raid56 btrfs_endio_raid56_helper [btrfs] >> RIP: 0010:[<ffffffff813a2fa8>] [<ffffffff813a2fa8>] generic_make_request_checks+0x198/0x5a0 >> Call Trace: >> [<ffffffff813a4cff>] ? generic_make_request+0xcf/0x290 >> [<ffffffff813a4c54>] generic_make_request+0x24/0x290 >> [<ffffffff813a4cff>] ? generic_make_request+0xcf/0x290 >> [<ffffffff813a4f2e>] submit_bio+0x6e/0x120 >> [<ffffffffa087279d>] ? page_in_rbio+0x4d/0x80 [btrfs] >> [<ffffffffa08737d0>] ? rbio_orig_end_io+0x80/0x80 [btrfs] >> [<ffffffffa0873e31>] finish_rmw+0x401/0x550 [btrfs] >> [<ffffffffa0874fc6>] validate_rbio_for_rmw+0x36/0x40 [btrfs] >> [<ffffffffa087504d>] raid_rmw_end_io+0x7d/0x90 [btrfs] >> [<ffffffff8139c536>] bio_endio+0x56/0x60 >> [<ffffffffa07e6e5c>] end_workqueue_fn+0x3c/0x40 [btrfs] >> [<ffffffffa08285bf>] btrfs_scrubparity_helper+0xef/0x610 [btrfs] >> [<ffffffffa0828b9e>] btrfs_endio_raid56_helper+0xe/0x10 [btrfs] >> [<ffffffff810ec8df>] process_one_work+0x2af/0x720 >> [<ffffffff810ec85b>] ? process_one_work+0x22b/0x720 >> [<ffffffff810ecd9b>] worker_thread+0x4b/0x4f0 >> [<ffffffff810ecd50>] ? process_one_work+0x720/0x720 >> [<ffffffff810ecd50>] ? process_one_work+0x720/0x720 >> [<ffffffff810f39d3>] kthread+0xf3/0x110 >> [<ffffffff810f38e0>] ? kthread_park+0x60/0x60 >> [<ffffffff81857647>] ret_from_fork+0x27/0x40 >> >> While in that case, target device can be destroyed at cancel time, >> leading to a user-after-free bug: >> >> Process A (dev-replace) | Process B(scrub) >> ---------------------------------------------------------------------- >> |(Any RW is OK) >> |scrub_setup_recheck_block() >> ||- btrfs_map_sblock() >> | Got a bbio with tgtdev >> btrfs_dev_replace_finishing() | >> |- btrfs_destory_dev_replace_tgtdev()| >> |- call_rcu(free_device) | >> |- __free_device() | >> |- kfree(device) | >> | Scrub worker: >> | Access bbio->stripes[], which >> | contains tgtdev. >> | This triggers general protection. >> > > We already have bio_counter to block device replace to avoid > use-after-free problem, is there any particular reason of not using > it? Thanks for pointing this out. And I found raid56 is already using bio_counter(). But we still trigger such use-after-free bug because bio_counter only ensure we won't delete/replace a device when there is still bio running. Here we are facing a difference situation, where we holds a pointer to btrfs_dev which can be freed since there is no pending bios for the whole fs. So here I'm afraid we need refcount based solution for each btrfs_dev structure, other than global bio_counter. Thanks, Qu > > Thanks, > > -liubo > >> The bug is mostly obvious for RAID5/6 since raid56 choose to keep old >> rbio and rbio->bbio for later steal, this hugely enlarged the race >> window and makes it much easier to trigger the bug. >> >> This patch introduces 'tgtdev_refs' and 'tgtdev_wait' for btrfs_device >> to wait for all its user released the target device. >> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >> --- >> fs/btrfs/dev-replace.c | 7 ++++++- >> fs/btrfs/volumes.c | 36 +++++++++++++++++++++++++++++++++++- >> fs/btrfs/volumes.h | 10 ++++++++++ >> 3 files changed, 51 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c >> index 5de280b9ad73..794a6a0bedf2 100644 >> --- a/fs/btrfs/dev-replace.c >> +++ b/fs/btrfs/dev-replace.c >> @@ -558,7 +558,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, >> rcu_str_deref(src_device->name), >> src_device->devid, >> rcu_str_deref(tgt_device->name)); >> - tgt_device->is_tgtdev_for_dev_replace = 0; >> tgt_device->devid = src_device->devid; >> src_device->devid = BTRFS_DEV_REPLACE_DEVID; >> memcpy(uuid_tmp, tgt_device->uuid, sizeof(uuid_tmp)); >> @@ -579,6 +578,12 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, >> >> btrfs_dev_replace_unlock(dev_replace, 1); >> >> + /* >> + * Only change is_tgtdev_for_dev_replace flag after all its >> + * users get released. >> + */ >> + wait_target_device(tgt_device); >> + tgt_device->is_tgtdev_for_dev_replace = 0; >> btrfs_rm_dev_replace_blocked(fs_info); >> >> btrfs_rm_dev_replace_remove_srcdev(fs_info, src_device); >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 3c3c69c0eee4..84db9fb22b7d 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -2064,6 +2064,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, >> WARN_ON(!tgtdev); >> mutex_lock(&fs_info->fs_devices->device_list_mutex); >> >> + wait_target_device(tgtdev); >> btrfs_sysfs_rm_device_link(fs_info->fs_devices, tgtdev); >> >> if (tgtdev->bdev) >> @@ -2598,6 +2599,8 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, >> device->is_tgtdev_for_dev_replace = 1; >> device->mode = FMODE_EXCL; >> device->dev_stats_valid = 1; >> + atomic_set(&device->tgtdev_refs, 0); >> + init_waitqueue_head(&device->tgtdev_wait); >> set_blocksize(device->bdev, 4096); >> device->fs_devices = fs_info->fs_devices; >> list_add(&device->dev_list, &fs_info->fs_devices->devices); >> @@ -2624,6 +2627,8 @@ void btrfs_init_dev_replace_tgtdev_for_resume(struct btrfs_fs_info *fs_info, >> tgtdev->sector_size = sectorsize; >> tgtdev->fs_info = fs_info; >> tgtdev->in_fs_metadata = 1; >> + atomic_set(&tgtdev->tgtdev_refs, 0); >> + init_waitqueue_head(&tgtdev->tgtdev_wait); >> } >> >> static noinline int btrfs_update_device(struct btrfs_trans_handle *trans, >> @@ -5302,6 +5307,32 @@ static struct btrfs_bio *alloc_btrfs_bio(int total_stripes, int real_stripes) >> return bbio; >> } >> >> +static void pin_bbio_target_device(struct btrfs_bio *bbio) >> +{ >> + int i; >> + >> + for (i = 0; i < bbio->num_stripes; i++) { >> + struct btrfs_device *device = bbio->stripes[i].dev; >> + >> + if (device->is_tgtdev_for_dev_replace) >> + atomic_inc(&device->tgtdev_refs); >> + } >> +} >> + >> +static void unpin_bbio_target_device(struct btrfs_bio *bbio) >> +{ >> + int i; >> + >> + for (i = 0; i < bbio->num_stripes; i++) { >> + struct btrfs_device *device = bbio->stripes[i].dev; >> + >> + if (device->is_tgtdev_for_dev_replace) { >> + atomic_dec(&device->tgtdev_refs); >> + wake_up(&device->tgtdev_wait); >> + } >> + } >> +} >> + >> void btrfs_get_bbio(struct btrfs_bio *bbio) >> { >> WARN_ON(!atomic_read(&bbio->refs)); >> @@ -5312,8 +5343,10 @@ void btrfs_put_bbio(struct btrfs_bio *bbio) >> { >> if (!bbio) >> return; >> - if (atomic_dec_and_test(&bbio->refs)) >> + if (atomic_dec_and_test(&bbio->refs)) { >> + unpin_bbio_target_device(bbio); >> kfree(bbio); >> + } >> } >> >> static int __btrfs_map_block(struct btrfs_fs_info *fs_info, >> @@ -5868,6 +5901,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info, >> bbio->stripes[0].physical = physical_to_patch_in_first_stripe; >> bbio->mirror_num = map->num_stripes + 1; >> } >> + pin_bbio_target_device(bbio); >> out: >> if (dev_replace_is_ongoing) { >> btrfs_dev_replace_clear_lock_blocking(dev_replace); >> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >> index 24ba6bc3ec34..702361182597 100644 >> --- a/fs/btrfs/volumes.h >> +++ b/fs/btrfs/volumes.h >> @@ -149,6 +149,10 @@ struct btrfs_device { >> /* Counter to record the change of device stats */ >> atomic_t dev_stats_ccnt; >> atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX]; >> + >> + /* To ensure we wait this target device before destroying it */ >> + atomic_t tgtdev_refs; >> + wait_queue_head_t tgtdev_wait; >> }; >> >> /* >> @@ -538,4 +542,10 @@ struct list_head *btrfs_get_fs_uuids(void); >> void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info); >> void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info); >> >> +static inline void wait_target_device(struct btrfs_device *tgtdev) >> +{ >> + if (!tgtdev || !tgtdev->is_tgtdev_for_dev_replace) >> + return; >> + wait_event(tgtdev->tgtdev_wait, atomic_read(&tgtdev->tgtdev_refs) == 0); >> +} >> #endif >> -- >> 2.11.0 >> >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 20, 2017 at 02:30:48PM +0800, Qu Wenruo wrote: > > > At 03/18/2017 10:12 AM, Liu Bo wrote: > > On Fri, Feb 03, 2017 at 04:20:23PM +0800, Qu Wenruo wrote: > > > When dev-replace and scrub are run at the same time, dev-replace can be > > > canceled by scrub. It's quite common for btrfs/069. > > > > > > The backtrace would be like: > > > general protection fault: 0000 [#1] SMP > > > Workqueue: btrfs-endio-raid56 btrfs_endio_raid56_helper [btrfs] > > > RIP: 0010:[<ffffffff813a2fa8>] [<ffffffff813a2fa8>] generic_make_request_checks+0x198/0x5a0 > > > Call Trace: > > > [<ffffffff813a4cff>] ? generic_make_request+0xcf/0x290 > > > [<ffffffff813a4c54>] generic_make_request+0x24/0x290 > > > [<ffffffff813a4cff>] ? generic_make_request+0xcf/0x290 > > > [<ffffffff813a4f2e>] submit_bio+0x6e/0x120 > > > [<ffffffffa087279d>] ? page_in_rbio+0x4d/0x80 [btrfs] > > > [<ffffffffa08737d0>] ? rbio_orig_end_io+0x80/0x80 [btrfs] > > > [<ffffffffa0873e31>] finish_rmw+0x401/0x550 [btrfs] > > > [<ffffffffa0874fc6>] validate_rbio_for_rmw+0x36/0x40 [btrfs] > > > [<ffffffffa087504d>] raid_rmw_end_io+0x7d/0x90 [btrfs] > > > [<ffffffff8139c536>] bio_endio+0x56/0x60 > > > [<ffffffffa07e6e5c>] end_workqueue_fn+0x3c/0x40 [btrfs] > > > [<ffffffffa08285bf>] btrfs_scrubparity_helper+0xef/0x610 [btrfs] > > > [<ffffffffa0828b9e>] btrfs_endio_raid56_helper+0xe/0x10 [btrfs] > > > [<ffffffff810ec8df>] process_one_work+0x2af/0x720 > > > [<ffffffff810ec85b>] ? process_one_work+0x22b/0x720 > > > [<ffffffff810ecd9b>] worker_thread+0x4b/0x4f0 > > > [<ffffffff810ecd50>] ? process_one_work+0x720/0x720 > > > [<ffffffff810ecd50>] ? process_one_work+0x720/0x720 > > > [<ffffffff810f39d3>] kthread+0xf3/0x110 > > > [<ffffffff810f38e0>] ? kthread_park+0x60/0x60 > > > [<ffffffff81857647>] ret_from_fork+0x27/0x40 > > > > > > While in that case, target device can be destroyed at cancel time, > > > leading to a user-after-free bug: > > > > > > Process A (dev-replace) | Process B(scrub) > > > ---------------------------------------------------------------------- > > > |(Any RW is OK) > > > |scrub_setup_recheck_block() > > > ||- btrfs_map_sblock() > > > | Got a bbio with tgtdev > > > btrfs_dev_replace_finishing() | > > > |- btrfs_destory_dev_replace_tgtdev()| > > > |- call_rcu(free_device) | > > > |- __free_device() | > > > |- kfree(device) | > > > | Scrub worker: > > > | Access bbio->stripes[], which > > > | contains tgtdev. > > > | This triggers general protection. > > > > > > > We already have bio_counter to block device replace to avoid > > use-after-free problem, is there any particular reason of not using > > it? > > Thanks for pointing this out. > > And I found raid56 is already using bio_counter(). > But we still trigger such use-after-free bug because bio_counter only ensure > we won't delete/replace a device when there is still bio running. > > Here we are facing a difference situation, where we holds a pointer to > btrfs_dev which can be freed since there is no pending bios for the whole > fs. > It's not just for in-flight bios, check btrfs_discard_extent(), anyone who expects target devices to be alive can use bio_counter to avoid the race against device replace. You could inc/dec bio_counter along with scrub_recover as it takes a pointer of bbio and maintains a reference. Thanks, -liubo > So here I'm afraid we need refcount based solution for each btrfs_dev > structure, other than global bio_counter. > > Thanks, > Qu > > > > Thanks, > > > > -liubo > > > > > The bug is mostly obvious for RAID5/6 since raid56 choose to keep old > > > rbio and rbio->bbio for later steal, this hugely enlarged the race > > > window and makes it much easier to trigger the bug. > > > > > > This patch introduces 'tgtdev_refs' and 'tgtdev_wait' for btrfs_device > > > to wait for all its user released the target device. > > > > > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > > > --- > > > fs/btrfs/dev-replace.c | 7 ++++++- > > > fs/btrfs/volumes.c | 36 +++++++++++++++++++++++++++++++++++- > > > fs/btrfs/volumes.h | 10 ++++++++++ > > > 3 files changed, 51 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c > > > index 5de280b9ad73..794a6a0bedf2 100644 > > > --- a/fs/btrfs/dev-replace.c > > > +++ b/fs/btrfs/dev-replace.c > > > @@ -558,7 +558,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, > > > rcu_str_deref(src_device->name), > > > src_device->devid, > > > rcu_str_deref(tgt_device->name)); > > > - tgt_device->is_tgtdev_for_dev_replace = 0; > > > tgt_device->devid = src_device->devid; > > > src_device->devid = BTRFS_DEV_REPLACE_DEVID; > > > memcpy(uuid_tmp, tgt_device->uuid, sizeof(uuid_tmp)); > > > @@ -579,6 +578,12 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, > > > > > > btrfs_dev_replace_unlock(dev_replace, 1); > > > > > > + /* > > > + * Only change is_tgtdev_for_dev_replace flag after all its > > > + * users get released. > > > + */ > > > + wait_target_device(tgt_device); > > > + tgt_device->is_tgtdev_for_dev_replace = 0; > > > btrfs_rm_dev_replace_blocked(fs_info); > > > > > > btrfs_rm_dev_replace_remove_srcdev(fs_info, src_device); > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > > index 3c3c69c0eee4..84db9fb22b7d 100644 > > > --- a/fs/btrfs/volumes.c > > > +++ b/fs/btrfs/volumes.c > > > @@ -2064,6 +2064,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, > > > WARN_ON(!tgtdev); > > > mutex_lock(&fs_info->fs_devices->device_list_mutex); > > > > > > + wait_target_device(tgtdev); > > > btrfs_sysfs_rm_device_link(fs_info->fs_devices, tgtdev); > > > > > > if (tgtdev->bdev) > > > @@ -2598,6 +2599,8 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, > > > device->is_tgtdev_for_dev_replace = 1; > > > device->mode = FMODE_EXCL; > > > device->dev_stats_valid = 1; > > > + atomic_set(&device->tgtdev_refs, 0); > > > + init_waitqueue_head(&device->tgtdev_wait); > > > set_blocksize(device->bdev, 4096); > > > device->fs_devices = fs_info->fs_devices; > > > list_add(&device->dev_list, &fs_info->fs_devices->devices); > > > @@ -2624,6 +2627,8 @@ void btrfs_init_dev_replace_tgtdev_for_resume(struct btrfs_fs_info *fs_info, > > > tgtdev->sector_size = sectorsize; > > > tgtdev->fs_info = fs_info; > > > tgtdev->in_fs_metadata = 1; > > > + atomic_set(&tgtdev->tgtdev_refs, 0); > > > + init_waitqueue_head(&tgtdev->tgtdev_wait); > > > } > > > > > > static noinline int btrfs_update_device(struct btrfs_trans_handle *trans, > > > @@ -5302,6 +5307,32 @@ static struct btrfs_bio *alloc_btrfs_bio(int total_stripes, int real_stripes) > > > return bbio; > > > } > > > > > > +static void pin_bbio_target_device(struct btrfs_bio *bbio) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < bbio->num_stripes; i++) { > > > + struct btrfs_device *device = bbio->stripes[i].dev; > > > + > > > + if (device->is_tgtdev_for_dev_replace) > > > + atomic_inc(&device->tgtdev_refs); > > > + } > > > +} > > > + > > > +static void unpin_bbio_target_device(struct btrfs_bio *bbio) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < bbio->num_stripes; i++) { > > > + struct btrfs_device *device = bbio->stripes[i].dev; > > > + > > > + if (device->is_tgtdev_for_dev_replace) { > > > + atomic_dec(&device->tgtdev_refs); > > > + wake_up(&device->tgtdev_wait); > > > + } > > > + } > > > +} > > > + > > > void btrfs_get_bbio(struct btrfs_bio *bbio) > > > { > > > WARN_ON(!atomic_read(&bbio->refs)); > > > @@ -5312,8 +5343,10 @@ void btrfs_put_bbio(struct btrfs_bio *bbio) > > > { > > > if (!bbio) > > > return; > > > - if (atomic_dec_and_test(&bbio->refs)) > > > + if (atomic_dec_and_test(&bbio->refs)) { > > > + unpin_bbio_target_device(bbio); > > > kfree(bbio); > > > + } > > > } > > > > > > static int __btrfs_map_block(struct btrfs_fs_info *fs_info, > > > @@ -5868,6 +5901,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info, > > > bbio->stripes[0].physical = physical_to_patch_in_first_stripe; > > > bbio->mirror_num = map->num_stripes + 1; > > > } > > > + pin_bbio_target_device(bbio); > > > out: > > > if (dev_replace_is_ongoing) { > > > btrfs_dev_replace_clear_lock_blocking(dev_replace); > > > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > > > index 24ba6bc3ec34..702361182597 100644 > > > --- a/fs/btrfs/volumes.h > > > +++ b/fs/btrfs/volumes.h > > > @@ -149,6 +149,10 @@ struct btrfs_device { > > > /* Counter to record the change of device stats */ > > > atomic_t dev_stats_ccnt; > > > atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX]; > > > + > > > + /* To ensure we wait this target device before destroying it */ > > > + atomic_t tgtdev_refs; > > > + wait_queue_head_t tgtdev_wait; > > > }; > > > > > > /* > > > @@ -538,4 +542,10 @@ struct list_head *btrfs_get_fs_uuids(void); > > > void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info); > > > void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info); > > > > > > +static inline void wait_target_device(struct btrfs_device *tgtdev) > > > +{ > > > + if (!tgtdev || !tgtdev->is_tgtdev_for_dev_replace) > > > + return; > > > + wait_event(tgtdev->tgtdev_wait, atomic_read(&tgtdev->tgtdev_refs) == 0); > > > +} > > > #endif > > > -- > > > 2.11.0 > > > > > > > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c index 5de280b9ad73..794a6a0bedf2 100644 --- a/fs/btrfs/dev-replace.c +++ b/fs/btrfs/dev-replace.c @@ -558,7 +558,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, rcu_str_deref(src_device->name), src_device->devid, rcu_str_deref(tgt_device->name)); - tgt_device->is_tgtdev_for_dev_replace = 0; tgt_device->devid = src_device->devid; src_device->devid = BTRFS_DEV_REPLACE_DEVID; memcpy(uuid_tmp, tgt_device->uuid, sizeof(uuid_tmp)); @@ -579,6 +578,12 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, btrfs_dev_replace_unlock(dev_replace, 1); + /* + * Only change is_tgtdev_for_dev_replace flag after all its + * users get released. + */ + wait_target_device(tgt_device); + tgt_device->is_tgtdev_for_dev_replace = 0; btrfs_rm_dev_replace_blocked(fs_info); btrfs_rm_dev_replace_remove_srcdev(fs_info, src_device); diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 3c3c69c0eee4..84db9fb22b7d 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2064,6 +2064,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, WARN_ON(!tgtdev); mutex_lock(&fs_info->fs_devices->device_list_mutex); + wait_target_device(tgtdev); btrfs_sysfs_rm_device_link(fs_info->fs_devices, tgtdev); if (tgtdev->bdev) @@ -2598,6 +2599,8 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, device->is_tgtdev_for_dev_replace = 1; device->mode = FMODE_EXCL; device->dev_stats_valid = 1; + atomic_set(&device->tgtdev_refs, 0); + init_waitqueue_head(&device->tgtdev_wait); set_blocksize(device->bdev, 4096); device->fs_devices = fs_info->fs_devices; list_add(&device->dev_list, &fs_info->fs_devices->devices); @@ -2624,6 +2627,8 @@ void btrfs_init_dev_replace_tgtdev_for_resume(struct btrfs_fs_info *fs_info, tgtdev->sector_size = sectorsize; tgtdev->fs_info = fs_info; tgtdev->in_fs_metadata = 1; + atomic_set(&tgtdev->tgtdev_refs, 0); + init_waitqueue_head(&tgtdev->tgtdev_wait); } static noinline int btrfs_update_device(struct btrfs_trans_handle *trans, @@ -5302,6 +5307,32 @@ static struct btrfs_bio *alloc_btrfs_bio(int total_stripes, int real_stripes) return bbio; } +static void pin_bbio_target_device(struct btrfs_bio *bbio) +{ + int i; + + for (i = 0; i < bbio->num_stripes; i++) { + struct btrfs_device *device = bbio->stripes[i].dev; + + if (device->is_tgtdev_for_dev_replace) + atomic_inc(&device->tgtdev_refs); + } +} + +static void unpin_bbio_target_device(struct btrfs_bio *bbio) +{ + int i; + + for (i = 0; i < bbio->num_stripes; i++) { + struct btrfs_device *device = bbio->stripes[i].dev; + + if (device->is_tgtdev_for_dev_replace) { + atomic_dec(&device->tgtdev_refs); + wake_up(&device->tgtdev_wait); + } + } +} + void btrfs_get_bbio(struct btrfs_bio *bbio) { WARN_ON(!atomic_read(&bbio->refs)); @@ -5312,8 +5343,10 @@ void btrfs_put_bbio(struct btrfs_bio *bbio) { if (!bbio) return; - if (atomic_dec_and_test(&bbio->refs)) + if (atomic_dec_and_test(&bbio->refs)) { + unpin_bbio_target_device(bbio); kfree(bbio); + } } static int __btrfs_map_block(struct btrfs_fs_info *fs_info, @@ -5868,6 +5901,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info, bbio->stripes[0].physical = physical_to_patch_in_first_stripe; bbio->mirror_num = map->num_stripes + 1; } + pin_bbio_target_device(bbio); out: if (dev_replace_is_ongoing) { btrfs_dev_replace_clear_lock_blocking(dev_replace); diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 24ba6bc3ec34..702361182597 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -149,6 +149,10 @@ struct btrfs_device { /* Counter to record the change of device stats */ atomic_t dev_stats_ccnt; atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX]; + + /* To ensure we wait this target device before destroying it */ + atomic_t tgtdev_refs; + wait_queue_head_t tgtdev_wait; }; /* @@ -538,4 +542,10 @@ struct list_head *btrfs_get_fs_uuids(void); void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info); void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info); +static inline void wait_target_device(struct btrfs_device *tgtdev) +{ + if (!tgtdev || !tgtdev->is_tgtdev_for_dev_replace) + return; + wait_event(tgtdev->tgtdev_wait, atomic_read(&tgtdev->tgtdev_refs) == 0); +} #endif
When dev-replace and scrub are run at the same time, dev-replace can be canceled by scrub. It's quite common for btrfs/069. The backtrace would be like: general protection fault: 0000 [#1] SMP Workqueue: btrfs-endio-raid56 btrfs_endio_raid56_helper [btrfs] RIP: 0010:[<ffffffff813a2fa8>] [<ffffffff813a2fa8>] generic_make_request_checks+0x198/0x5a0 Call Trace: [<ffffffff813a4cff>] ? generic_make_request+0xcf/0x290 [<ffffffff813a4c54>] generic_make_request+0x24/0x290 [<ffffffff813a4cff>] ? generic_make_request+0xcf/0x290 [<ffffffff813a4f2e>] submit_bio+0x6e/0x120 [<ffffffffa087279d>] ? page_in_rbio+0x4d/0x80 [btrfs] [<ffffffffa08737d0>] ? rbio_orig_end_io+0x80/0x80 [btrfs] [<ffffffffa0873e31>] finish_rmw+0x401/0x550 [btrfs] [<ffffffffa0874fc6>] validate_rbio_for_rmw+0x36/0x40 [btrfs] [<ffffffffa087504d>] raid_rmw_end_io+0x7d/0x90 [btrfs] [<ffffffff8139c536>] bio_endio+0x56/0x60 [<ffffffffa07e6e5c>] end_workqueue_fn+0x3c/0x40 [btrfs] [<ffffffffa08285bf>] btrfs_scrubparity_helper+0xef/0x610 [btrfs] [<ffffffffa0828b9e>] btrfs_endio_raid56_helper+0xe/0x10 [btrfs] [<ffffffff810ec8df>] process_one_work+0x2af/0x720 [<ffffffff810ec85b>] ? process_one_work+0x22b/0x720 [<ffffffff810ecd9b>] worker_thread+0x4b/0x4f0 [<ffffffff810ecd50>] ? process_one_work+0x720/0x720 [<ffffffff810ecd50>] ? process_one_work+0x720/0x720 [<ffffffff810f39d3>] kthread+0xf3/0x110 [<ffffffff810f38e0>] ? kthread_park+0x60/0x60 [<ffffffff81857647>] ret_from_fork+0x27/0x40 While in that case, target device can be destroyed at cancel time, leading to a user-after-free bug: Process A (dev-replace) | Process B(scrub) ---------------------------------------------------------------------- |(Any RW is OK) |scrub_setup_recheck_block() ||- btrfs_map_sblock() | Got a bbio with tgtdev btrfs_dev_replace_finishing() | |- btrfs_destory_dev_replace_tgtdev()| |- call_rcu(free_device) | |- __free_device() | |- kfree(device) | | Scrub worker: | Access bbio->stripes[], which | contains tgtdev. | This triggers general protection. The bug is mostly obvious for RAID5/6 since raid56 choose to keep old rbio and rbio->bbio for later steal, this hugely enlarged the race window and makes it much easier to trigger the bug. This patch introduces 'tgtdev_refs' and 'tgtdev_wait' for btrfs_device to wait for all its user released the target device. Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- fs/btrfs/dev-replace.c | 7 ++++++- fs/btrfs/volumes.c | 36 +++++++++++++++++++++++++++++++++++- fs/btrfs/volumes.h | 10 ++++++++++ 3 files changed, 51 insertions(+), 2 deletions(-)