Re: [PATCH 1/3] btrfs: tree-checker: Add EXTENT_ITEM and METADATA_ITEM check

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2019/7/29 下午9:42, Nikolay Borisov wrote:
[...]
>> +
>> +	/* key->offset is tree level for METADATA_ITEM_KEY */
>> +	if (key->type == BTRFS_METADATA_ITEM_KEY) {
>> +		if (key->offset >= BTRFS_MAX_LEVEL) {
>
> make it a compound statement:
>
> if key->type && key->offset. The extra if doesn't bring anything...

Right, that's the case.
I though there would be more checks, but turns out there is only one check.

>
>> +			extent_err(leaf, slot,
>> +				"invalid tree level, have %llu expect [0, %u]",
>> +				   key->offset, BTRFS_MAX_LEVEL - 1);
>> +			return -EUCLEAN;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * EXTENT/METADATA_ITEM is consistent of:
>> +	 * 1) One btrfs_extent_item
>> +	 *    Records the total refs, type and generation of the extent.
>> +	 *
>> +	 * 2) One btrfs_tree_block_info (for EXTENT_ITEM and tree backref only)
>> +	 *    Records the first key and level of the tree block.
>> +	 *
>> +	 * 2) *Zero* or more btrfs_extent_inline_ref(s)
>> +	 *    Each inline ref has one btrfs_extent_inline_ref shows:
>> +	 *    2.1) The ref type, one of the 4
>> +	 *         TREE_BLOCK_REF	Tree block only
>> +	 *         SHARED_BLOCK_REF	Tree block only
>> +	 *         EXTENT_DATA_REF	Data only
>> +	 *         SHARED_DATA_REF	Data only
>> +	 *    2.2) Ref type specific data
>> +	 *         Either using btrfs_extent_inline_ref::offset, or specific
>> +	 *         data structure.
>> +	 */
>> +	if (item_size < sizeof(*ei)) {
>> +		extent_err(leaf, slot,
>> +			   "invalid item size, have %u expect >= %lu",
>> +			   item_size, sizeof(*ei));
>> +		return -EUCLEAN;
>> +	}
>> +	end = item_size + btrfs_item_ptr_offset(leaf, slot);
>> +
>> +	/* Checks against extent_item */
>> +	ei = btrfs_item_ptr(leaf, slot, struct btrfs_extent_item);
>> +	flags = btrfs_extent_flags(leaf, ei);
>> +	total_refs = btrfs_extent_refs(leaf, ei);
>> +	generation = btrfs_extent_generation(leaf, ei);
>> +	if (generation > btrfs_super_generation(fs_info->super_copy) + 1) {
>> +		extent_err(leaf, slot,
>> +			"invalid generation, have %llu expect <= %llu",
>
> nit: I find the '<= [number]' somewhat confusing, wouldn't it be better
> if it's spelled our e.g 'expecting less than or equal than [number]'.
> Might be just me.

I normally prefer the "[start, end]" notion, but sometimes even myself
can't always keep the format the same.

Furthermore, I can't find a minimal value to use [start, end] notion.
Maybe (0, end] would be more suitable here?

>
>> +			   generation,
>> +			   btrfs_super_generation(fs_info->super_copy) + 1);
>> +		return -EUCLEAN;
>> +	}
>> +	if (!is_power_of_2(flags & (BTRFS_EXTENT_FLAG_DATA |
>> +				    BTRFS_EXTENT_FLAG_TREE_BLOCK))) {
>> +		extent_err(leaf, slot,
>> +		"invalid extent flag, have 0x%llx expect 1 bit set in 0x%llx",
>> +			flags, BTRFS_EXTENT_FLAG_DATA |
>> +			BTRFS_EXTENT_FLAG_TREE_BLOCK);
>> +		return -EUCLEAN;
>> +	}
>> +	is_tree_block = !!(flags & BTRFS_EXTENT_FLAG_TREE_BLOCK);
>> +	if (is_tree_block && key->type == BTRFS_EXTENT_ITEM_KEY &&
>> +	    key->offset != fs_info->nodesize) {
>> +		extent_err(leaf, slot,
>> +			   "invalid extent length, have %llu expect %u",
>> +			   key->offset, fs_info->nodesize);
>> +		return -EUCLEAN;
>> +	}
>> +	if (!is_tree_block) {
>> +		if (key->type != BTRFS_EXTENT_ITEM_KEY) {
>> +			extent_err(leaf, slot,
>> +		"invalid key type, have %u expect %u for data backref",
>> +				   key->type, BTRFS_EXTENT_ITEM_KEY);
>> +			return -EUCLEAN;
>> +		}
>> +		if (!IS_ALIGNED(key->offset, fs_info->sectorsize)) {
>> +			extent_err(leaf, slot,
>> +			"invalid extent length, have %llu expect aliged to %u",
>> +				   key->offset, fs_info->sectorsize);
>> +			return -EUCLEAN;
>> +		}
>> +	}
>
> unify the two is_tree_block/!is_tree_block either in:
>
> if (is_tree_block) {
> 	if (keypt->type = EXTENT_ITEM_KEY && offset != nodesize {
> 		foo;
> 	}
> } else {
> 	bar;
> }
>

Right, that's better.

> or
>
> if (is_tree_block && key->type == BTRFS_EXTENT_ITEM_KEY ..) {
>
> } else if (!is_tree_block) {
> bar
> }
>
>> +	ptr = (u64)(struct btrfs_extent_item *)(ei + 1);
>> +
>> +	/* Check the special case of btrfs_tree_block_info */
>> +	if (is_tree_block && key->type != BTRFS_METADATA_ITEM_KEY) {
>> +		struct btrfs_tree_block_info *info;
>> +
>> +		info = (struct btrfs_tree_block_info *)ptr;
>> +		if (btrfs_tree_block_level(leaf, info) >= BTRFS_MAX_LEVEL) {
>> +			extent_err(leaf, slot,
>> +			"invalid tree block info level, have %u expect [0, %u)",
>
> nit: Strictly speaking using [0, 7) is wrong, because ')' already
> implies an open interval. Since we have 8 levels 0/7 inclusive this
> means the correct way to print it would be [0, 7] or [0, 8).

It's not that rare I misuses such notion. [0, 7] would be the case.

>
>
>> +				   btrfs_tree_block_level(leaf, info),
>> +				   BTRFS_MAX_LEVEL - 1);
>> +			return -EUCLEAN;
>> +		}
>> +		ptr = (u64)(struct btrfs_tree_block_info *)(info + 1);
>> +	}
>> +
>> +	/* Check inline refs */
>> +	while (ptr < end) {
>> +		struct btrfs_extent_inline_ref *iref;
>> +		struct btrfs_extent_data_ref *dref;
>> +		struct btrfs_shared_data_ref *sref;
>> +		u64 dref_offset;
>> +		u64 inline_offset;
>> +		u8 inline_type;
>> +
>> +		if (ptr + sizeof(*iref) > end) {
>> +			extent_err(leaf, slot,
>> +	"invalid item size, size too small, ptr %llu end %llu",
>> +				   ptr, end);
>> +			goto err;
>> +		}
>> +		iref = (struct btrfs_extent_inline_ref *)ptr;
>> +		inline_type = btrfs_extent_inline_ref_type(leaf, iref);
>> +		inline_offset = btrfs_extent_inline_ref_offset(leaf, iref);
>> +		if (ptr + btrfs_extent_inline_ref_size(inline_type) > end) {
>> +			extent_err(leaf, slot,
>> +	"invalid item size, size too small, ptr %llu end %llu",
>
> Make that text explicit:
>
> "inline ref item overflows extent item" or some such.

OK, I'll use this expression.

Thanks,
Qu

[...]
>> +
>> +	/* Finally, check the inline refs against total refs */
>> +	if (total_refs < inline_refs) {
>
> nit: if (inline_refs > total_refs) {} looks saner to me.
>
>
>
>> +		extent_err(leaf, slot,
>> +			"invalid extent refs, have %llu expect >= %llu",
>> +			   total_refs, inline_refs);
>> +		goto err;
>> +	}
>> +	return 0;
>> +err:
>> +	return -EUCLEAN;
>> +}
>> +
>>  /*
>>   * Common point to switch the item-specific validation.
>>   */
>> @@ -937,6 +1182,10 @@ static int check_leaf_item(struct extent_buffer *leaf,
>>  	case BTRFS_ROOT_ITEM_KEY:
>>  		ret = check_root_item(leaf, key, slot);
>>  		break;
>> +	case BTRFS_EXTENT_ITEM_KEY:
>> +	case BTRFS_METADATA_ITEM_KEY:
>> +		ret = check_extent_item(leaf, key, slot);
>> +		break;
>>  	}
>>  	return ret;
>>  }
>>