| Message ID | 20200304161830.2360-3-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | relocation error handling fixes | expand |
On 4.03.20 г. 18:18 ч., Josef Bacik wrote: > We previously were checking if the root had a dead root before accessing > root->reloc_root in order to avoid a UAF type bug. However this > scenario happens after we've unset the reloc control, so we would have > been saved if we'd simply checked for fs_info->reloc_control. At this > point during relocation we no longer need to be creating new reloc > roots, so simply move this check above the reloc_root checks to avoid > any future races and confusion. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Doesn't this patch essentially obviate the reloc_root_is_dead. W.r.t ->reloc_ctl it's important to note that it's being set under reloc_mutex which this function is also called under so we are guaranteed consistent value. > --- > fs/btrfs/relocation.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 507361e99316..2141519a9dd0 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -1527,6 +1527,10 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans, > int clear_rsv = 0; > int ret; > > + if (!rc || !rc->create_reloc_tree || > + root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) > + return 0; > + > /* > * The subvolume has reloc tree but the swap is finished, no need to > * create/update the dead reloc tree > @@ -1540,10 +1544,6 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans, > return 0; > } > > - if (!rc || !rc->create_reloc_tree || > - root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) > - return 0; > - > if (!trans->reloc_reserved) { > rsv = trans->block_rsv; > trans->block_rsv = rc->block_rsv; >
On 3/4/20 1:44 PM, Nikolay Borisov wrote: > > > On 4.03.20 г. 18:18 ч., Josef Bacik wrote: >> We previously were checking if the root had a dead root before accessing >> root->reloc_root in order to avoid a UAF type bug. However this >> scenario happens after we've unset the reloc control, so we would have >> been saved if we'd simply checked for fs_info->reloc_control. At this >> point during relocation we no longer need to be creating new reloc >> roots, so simply move this check above the reloc_root checks to avoid >> any future races and confusion. >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > Doesn't this patch essentially obviate the reloc_root_is_dead. W.r.t > ->reloc_ctl it's important to note that it's being set under reloc_mutex > which this function is also called under so we are guaranteed consistent > value. > Yes it does, but I want to keep the cleanups separate from the fixes. I threw this in here because it's more of a correctness/fix than a cleanup. Thanks, Josef
On 2020/3/5 上午12:18, Josef Bacik wrote: > We previously were checking if the root had a dead root before accessing > root->reloc_root in order to avoid a UAF type bug. However this > scenario happens after we've unset the reloc control, so we would have > been saved if we'd simply checked for fs_info->reloc_control. At this > point during relocation we no longer need to be creating new reloc > roots, so simply move this check above the reloc_root checks to avoid > any future races and confusion. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Right, for btrfs_init_reloc_root() checking rc before accessing root is fine. So, Reviewed-by: Qu Wenruo <wqu@suse.com> Although other location, like btrfs_reloc_post_snapshot() still needs such check, as we have a window where just merged reloc roots but not yet unset the reloc control. Thanks, Qu > --- > fs/btrfs/relocation.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 507361e99316..2141519a9dd0 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -1527,6 +1527,10 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans, > int clear_rsv = 0; > int ret; > > + if (!rc || !rc->create_reloc_tree || > + root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) > + return 0; > + > /* > * The subvolume has reloc tree but the swap is finished, no need to > * create/update the dead reloc tree > @@ -1540,10 +1544,6 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans, > return 0; > } > > - if (!rc || !rc->create_reloc_tree || > - root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) > - return 0; > - > if (!trans->reloc_reserved) { > rsv = trans->block_rsv; > trans->block_rsv = rc->block_rsv; >
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 507361e99316..2141519a9dd0 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -1527,6 +1527,10 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans, int clear_rsv = 0; int ret; + if (!rc || !rc->create_reloc_tree || + root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) + return 0; + /* * The subvolume has reloc tree but the swap is finished, no need to * create/update the dead reloc tree @@ -1540,10 +1544,6 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans, return 0; } - if (!rc || !rc->create_reloc_tree || - root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) - return 0; - if (!trans->reloc_reserved) { rsv = trans->block_rsv; trans->block_rsv = rc->block_rsv;
We previously were checking if the root had a dead root before accessing root->reloc_root in order to avoid a UAF type bug. However this scenario happens after we've unset the reloc control, so we would have been saved if we'd simply checked for fs_info->reloc_control. At this point during relocation we no longer need to be creating new reloc roots, so simply move this check above the reloc_root checks to avoid any future races and confusion. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/relocation.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)