Re: [PATCH] btrfs: Validate child tree block's level and first key

[Nikolay Borisov <n.borisov.lkml@gmail.com>]

On 22.03.2018 16:26, Qu Wenruo wrote:
> 
> 
> On 2018年03月22日 22:20, Nikolay Borisov wrote:
>>
>>
>> On 22.03.2018 16:17, Qu Wenruo wrote:
>>>
>>>
>>> On 2018年03月22日 22:00, David Sterba wrote:
>>>> On Thu, Mar 22, 2018 at 09:53:46PM +0800, Qu Wenruo wrote:
>>>>>>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>>>>>>> index 26484648d090..3866b8ab20f1 100644
>>>>>>> --- a/fs/btrfs/backref.c
>>>>>>> +++ b/fs/btrfs/backref.c
>>>>>>> @@ -738,7 +738,8 @@ static int add_missing_keys(struct btrfs_fs_info *fs_info,
>>>>>>>  		BUG_ON(ref->key_for_search.type);
>>>>>>>  		BUG_ON(!ref->wanted_disk_byte);
>>>>>>>  
>>>>>>> -		eb = read_tree_block(fs_info, ref->wanted_disk_byte, 0);
>>>>>>> +		eb = read_tree_block(fs_info, ref->wanted_disk_byte, 0, NULL,
>>>>>>> +				     0);
>>>>>>
>>>>>> Please add 2nd function that will take the extended parameters and
>>>>>> keep read_tree_block as is.
>>>>>
>>>>> So for any new caller of read_tree_block(), reviewer is the last person
>>>>> to info the author to use these parameters for safety check?
>>>>>
>>>>> And in fact, the old function should be avoid if possible, I think the
>>>>> new parameters act as a pretty good sign to make any caller double think
>>>>> about this.
>>>>
>>>> I saw half of the new parameters were just 0, NULL, so this looks like a
>>>> lot of code churn and I haven't looked closer if there's a chance to
>>>> fill the parameters in all callsites. So if it's a matter of adding them
>>>> incrementally then fine.
>>>>
>>> I'm afraid some of the call sites (ones I left with NULL, 0) are unable
>>> to pass the new parameters by its nature.
>>>
>>> Such callers include:
>>> 1) Tree root
>>>    Just @bytenr and @gen from ROOT_ITEM. No @first_key.
>>>
>>> 2) Backref walker for FULL_BACKREF
>>>    Only parent bytenr, no extra info on @first_key.
>>>
>>> But despite of such call sites, every top-down reader should grab first
>>> key and level. (And so I did in the patch).
>>>
>>> BTW, about half of the read_tree_block() callers are using the new
>>> parameters.
>>> So a new function seems a little embarrassing here.
>>
>>
>> Is it possible to centralise those checks in the read tree verifier,
>> rather than sprinkling them around the code?
> 
> The problem is, tree checker can only handle things *inside* a
> leaf/node, nothing can go beyond leaf/node boundary.
> 
> And for current check, we need a top-down pointer (nodeptr, which has
> bytenr, generation, first key along with the level) to do the
> verification, so that's the reason we can't put it into tree-checker.
> 
> (Sorry I forgot to add this explanation, and I didn't find better solution)

That's fine, but please put at least a sentence hinting at that in the
change log.

> 
> Thanks,
> Qu
> 
>>
>>>
>>> Thanks,
>>> Qu
>>>
>> --
>> 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
>>
>