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:39, Qu Wenruo wrote:
> 
> 
> On 2018年01月19日 18:21, Nikolay Borisov wrote:
>>
>>
>> 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 ?
> 
> This sounds reasonable, but it may end up as replacing NULL to
> PTR_ERR(-ENOENT).

Well in the case of check_dir_conflict it makes a whole lot of
difference. Currently if verify_dir_item fails then we propagate NULL up
to check_dir_conflict where we have the following error handling:

if (IS_ERR(dir_item)) {
                ret = PTR_ERR(dir_item);

                goto out;

        }

        if (dir_item) {

                ret = -EEXIST;

                goto out;

        }

so NULL won't really trigger anything, OTOH if we return an error and
IS_ERR triggers we won't add a duplicate item.

Be that as it may, I still think your patch as-is with modified commit
message is a good enough fix.

> Not much different for callers though.
> 
> Thanks,
> Qu
> 
>> </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
>>>>
>>>
>> --
>> 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
>>
>