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;
>> }
>>
