Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Nikolay Borisov <nborisov@suse.com>]

On 19.01.2018 12:03, Qu Wenruo wrote:
> 
> 
> On 2018年01月19日 17:40, Nikolay Borisov wrote:
>>
>>
>> On 19.01.2018 09:25, Qu Wenruo wrote:
>>> btrfs_match_dir_item_name() will check if its filetype is valid before
>>> doing search, this makes btrfs-progs unable to locate and remove invalid
>>> dir_index for btrfs_unlink().>
>>> This function only affects btrfs_link() and btrfs_unlink() in upper
>>> layer, and normal check can find invalid filetype by itself.
>>
>> There is no function btrfs_link in btrfs-progs, there is, however,
>> btrfs_add_link did you mean that function?
> 
> Yep, sorry for the wrong name.
> 
>> However, it doesn't seem to
>> use verify_dir_item hence the check you are removing. I think this part
>> of the commit log should be more precise. Also it's not really clear
>> what you mean by "normal check" in this sentence.
> 
> I skipped several function calls.
> 
> btrfs_add_link()
> |- check_dir_conflict
>    |- btrfs_lookup_dir_index() / btrfs_lookup_dir_item()
>       |- btrfs_match_dir_item_name()
>          |- verify_dir_item()
> 
> And for btrfs_add_link() without checking invalid filetype, we can avoid
> case like inserting duplicated DIR_ITEM/DIR_INDEX to avoid further
> screwing up things.

Right, since if verify_dir_item fails we just return NULL as opposed to
an error from btrfs_match_dir_item_name.

<offtopic>
Hm, so is it really possible to get a result for (key DIR_ITEM offset)
and have btrfs_match_dir_item_name fail for that result?

What I'm getting is that if btrfs_Search_slot returns 0 for such a query
irrespective of whether the string name matches or not we should return
some error code other than NULL ?
</offtopic>


> 
> For btrfs_unlink() it will be different story, as btrfs_unlink() can
> locate the conflicting but invalid DIR_ITEM/DIR_INDEX and remove it,
> allow us to fix the problem.
> 
> And by "normal check" I mean btrfs check.
> 
> I'll add the call trace to it.
> 
>>>
>>> So remove the filetype check is completely safe in this case, and will
>>> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.
>>
>> So the problem is that since btrfs_unlink calls verify_item and the
>> latter has the filetype check in case of wrong filetype (corruption)
>> verify_dir_item fails, hence we cannot perform the unlink? If my
>> understanding is correct how about something like:
>>
>>
>> If we have a corrupted dir item and enough information to repair it we
>> need to first delete the old/corrupted version and then insert a new
>> one.
>> However, btrfs_unlink calls btrfs_match_dir_item_name to locate the
>> offending dir item for deletion. The latter, in turn, uses
>> verify_dir_item which checks if the value for DIR item's type is sane.
>> In case of a corrupted type value then verify_dir_item will fail which
>> in turn will prevent btrfs_unlink from deleting the offending item.
> 
> Your understanding is completely right, and much shorter than my planned
> explanation.
> 
> I'll take it with extra call trace.
> 
> Thanks,
> Qu
> 
>>
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  dir-item.c | 6 ------
>>>  1 file changed, 6 deletions(-)
>>>
>>> diff --git a/dir-item.c b/dir-item.c
>>> index 462546c0eaf4..e0a0ab4d7a5d 100644
>>> --- a/dir-item.c
>>> +++ b/dir-item.c
>>> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>>>  	u16 namelen = BTRFS_NAME_LEN;
>>>  	u8 type = btrfs_dir_type(leaf, dir_item);
>>>  
>>> -	if (type >= BTRFS_FT_MAX) {
>>> -		fprintf(stderr, "invalid dir item type: %d\n",
>>> -		       (int)type);
>>> -		return 1;
>>> -	}
>>> -
>>>  	if (type == BTRFS_FT_XATTR)
>>>  		namelen = XATTR_NAME_MAX;
>>>  
>>>
>> --
>> 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
>>
>