Re: [PATCH RFC] btrfs-progs: map-logical: look at next leaf if slot > items

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

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



On 2018年06月08日 03:57, james harvey wrote:
> "On Thu, Jun 7, 2018 at 3:26 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>> On 2018年06月07日 15:19, james harvey wrote:
>>> On Thu, Jun 7, 2018 at 12:44 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>>> On 2018年06月07日 11:33, james harvey wrote:
>>>>> * btrfs_item_key_to_cpu sets key to (10955960320, BTRFS_EXTENT_ITEM_KEY, 4096)
>>>>> !!! Bonus bug, LEFT FOR READER.  Why is this item #197, 5 items before the 203
>>>>>     given?  I think no bounds checking causes a buffer over-read here.
>>>>>     btrfs_item_key_to_cpu() calls btrfs_item_key(), which uses the macro
>>>>>     read_eb_member() to call read_extent_buffer() which memcpy's using an out
>>>>>     of range index, at least for this leaf.
>>>>
>>>> Here the key you got could be garbage.
>>>> Either the leaf has enough free space, then you got pending zero into
>>>> the key, or the leaf is full, you got some item data into the key.
>>>>
>>>> Either way, the key should be read/trusted at all.
>>>
>>> Is the group's preference to leave things like bounds checking to the
>>> caller?
>>
>> The problem here is, btrfs_search_slot() is not only called for
>> read_only search, but also for insert.
>>
>> For insert usage, such returned path is completely valid and in fact
>> could be pretty useful.
> 
> I don't mean for btrfs_search_slot() behavior to change, since it is
> called for inserting.  I mean btrfs_item_key_to_cpu() behavior could
> change.

Makes sense!

An nice point to enhance!

> 
>>>  (I get the merit to that, avoiding redundant checks.)  My
>>> normal style would be to have read_extent_buffer() fail if start + len
>>> is out of bounds.
>>
>> In this case, it's not out-of-boundary at all, and the invalid key you
>> read out if from the last item, which could contain a valid key.
>>
>> For leaf, btrfs puts btrfs items pointers at the head, and the items
>> data at the tail.
>>
>> Thus for this case, your invalid key slot is just in the middle of the
>> leaf, making read_extent_buffer() unable to detect anything wrong.
>>
>> Thanks,
>> Qu
> 
> You're right.  read_extent_buffer() doesn't have the information to
> look at this.
> 
> And, "buffer over-read error" was probably the wrong term.  I mean in
> the sense that it has 203 valid values, and is being asked for the
> 204'th item.
> 
> btrfs_item_key() could error on
> 
> (nr > btrfs_header_nritems(eb))
> 
> This defensive programming would cause redundant checks, if the caller
> checks this too as they are supposed to, so I understand if the group
> doesn't swing that far on defensive programming.
> 
> Although, perhaps in such situation, btrfs_item_key() could
> automatically call btrfs_next_leaf().

This introduce extra return value, and for things like btrfs_item_key(),
we need to modify tons of call sites.

But at least, I could check if doing extra check (mostly a WARN_ON()) in
btrfs_item_key() is valid.

Thanks,
Qu

>  I'm thinking this might not be
> desired behavior everywhere, but just in case it is, mentioning it
> because then btrfs_item_key() could handle all of this, not allowing
> the caller to shoot their own foot, without redundant checks.
> --
> 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: 488 bytes --]