Re: [PATCH v8 12/17] btrfs: move existing remaps before relocating block group

[Chris Mason <clm@meta.com>]

Mark Harmstone <mark@harmstone.com> wrote:
> If when relocating a block group we find that `remap_bytes` > 0 in its
> block group item, that means that it has been the destination block
> group for another that has been remapped.
> 
> We need to seach the remap tree for any remap backrefs within this
> range, and move the data to a third block group. This is because
> otherwise btrfs_translate_remap() could end up following an unbounded
> chain of remaps, which would only get worse over time.
> 
> We only relocate one block group at a time, so `remap_bytes` will only
> ever go down while we are doing this. Once we're finished we set the
> REMAPPED flag on the block group, which will permanently prevent any
> other data from being moved to within it.

Hi everyone,

One more AI review from linux-next, this looks valid to me:

> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c

[ ... ]

> +static int move_existing_remap(struct btrfs_fs_info *fs_info,
> +			       struct btrfs_path *path,
> +			       struct btrfs_block_group *bg, u64 new_addr,
> +			       u64 length, u64 old_addr)
> +{

[ ... ]

> +	dest_bg = btrfs_lookup_block_group(fs_info, dest_addr);
> +
> +	adjust_block_group_remap_bytes(trans, dest_bg, dest_length);
> +
> +	mutex_lock(&dest_bg->free_space_lock);
> +	bg_needs_free_space = test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
> +				       &dest_bg->runtime_flags);
> +	mutex_unlock(&dest_bg->free_space_lock);
> +	btrfs_put_block_group(dest_bg);
> +
> +	if (bg_needs_free_space) {
> +		ret = btrfs_add_block_group_free_space(trans, dest_bg);
                                                         ^^^^^^^

Is there a use-after-free here? btrfs_put_block_group() is called above,
which can free the block group if the refcount drops to zero. Then
btrfs_add_block_group_free_space() dereferences dest_bg by calling
mutex_lock(&block_group->free_space_lock).

Perhaps the btrfs_put_block_group() call should be moved after the
conditional block that uses dest_bg?

> +		if (unlikely(ret))
> +			goto end;
> +	}

[ ... ]

> +static int move_existing_remaps(struct btrfs_fs_info *fs_info,
> +				struct btrfs_block_group *bg,
> +				struct btrfs_path *path)
> +{
> +	int ret;
> +	struct btrfs_key key;
> +	struct extent_buffer *leaf;
> +	struct btrfs_remap_item *remap;
> +	u64 old_addr;
> +
> +	/* Look for backrefs in remap tree. */
> +	while (bg->remap_bytes > 0) {
> +		key.objectid = bg->start;
> +		key.type = BTRFS_REMAP_BACKREF_KEY;
> +		key.offset = 0;
> +
> +		ret = btrfs_search_slot(NULL, fs_info->remap_root, &key, path, 0, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		leaf = path->nodes[0];
> +
> +		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
> +			ret = btrfs_next_leaf(fs_info->remap_root, path);

[ ... ]

> +			leaf = path->nodes[0];
> +		}
> +
> +		btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
> +
> +		if (key.type != BTRFS_REMAP_BACKREF_KEY) {
> +			path->slots[0]++;

When the slot is incremented here because the key type does not match,
should the key be re-read from the new slot? The code below reads
the remap item from path->slots[0] (the incremented slot), but then
passes key.objectid and key.offset (from the old slot) to
move_existing_remap().

If the key at the new slot has different objectid/offset values than
the previous slot, this would pass mismatched address and length values
to move_existing_remap().

> +
> +			if (path->slots[0] >= btrfs_header_nritems(leaf)) {
> +				ret = btrfs_next_leaf(fs_info->remap_root, path);

[ ... ]

> +				leaf = path->nodes[0];
> +			}
> +		}
> +
> +		remap = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_remap_item);
> +		old_addr = btrfs_remap_address(leaf, remap);
> +
> +		btrfs_release_path(path);
> +
> +		ret = move_existing_remap(fs_info, path, bg, key.objectid,
> +					  key.offset, old_addr);

[ ... ]