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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2867 bytes --]



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)

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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]