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). 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 <[email protected]> >>>> --- >>>> 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 [email protected] >>> 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 [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html >
signature.asc
Description: OpenPGP digital signature
