On 2018年06月07日 12:20, Su Yue wrote:
>
>
> On 06/07/2018 11:33 AM, james harvey wrote:
>> =====[ NOTE ]=====
>>
>> I think I found a buffer over-read error that will come up other places, unless
>> EACH caller checks bounds themselves. See "Bonus bug, LEFT FOR READER" below.
>>
>> =====[ PROBLEM ]=====
>>
>> Using btrfs-progs v4.16:
>>
>> No extent found at range [10955980800,10955984896)
>>
>> But, this extent exists. btrfs-debug-tree shows:
>> ...
>> extent tree key (EXTENT_TREE ROOT_ITEM 0)
>> ...
>> leaf 316456960 items 203 free space 3697 generation 84225 owner EXTENT_TREE
>> ...
>> item 197 key (10955931648 EXTENT_ITEM 28672) itemoff 8957 itemsize 37
>> refs 1 gen 62656 flags DATA
>> shared data backref parent 142622720 count 1
>> item 198 key (10955960320 EXTENT_ITEM 4096) itemoff 8920 itemsize 37
>> refs 1 gen 62656 flags DATA
>> shared data backref parent 142622720 count 1
>> item 199 key (10955964416 EXTENT_ITEM 4096) itemoff 8883 itemsize 37
>> refs 1 gen 62656 flags DATA
>> shared data backref parent 142622720 count 1
>> item 200 key (10955968512 EXTENT_ITEM 4096) itemoff 8846 itemsize 37
>> refs 1 gen 62656 flags DATA
>> shared data backref parent 142622720 count 1
>> item 201 key (10955972608 EXTENT_ITEM 4096) itemoff 8809 itemsize 37
>> refs 1 gen 62656 flags DATA
>> shared data backref parent 142655488 count 1
>> item 202 key (10955976704 EXTENT_ITEM 4096) itemoff 8772 itemsize 37
>> refs 1 gen 62656 flags DATA
>> shared data backref parent 142655488 count 1
>> ...
>> leaf 316489728 items 208 free space 3387 generation 84225 owner EXTENT_TREE
>> ...
>> item 0 key (10955980800 EXTENT_ITEM 4096) itemoff 16246 itemsize 37
>> refs 1 gen 62656 flags DATA
>> shared data backref parent 128958464 count 1
>> ...
>> checksum tree key (CSUM_TREE ROOT_ITEM 0)
>> ...
>> item 412 key (EXTENT_CSUM EXTENT_CSUM 10955980800) itemoff 10647
>> itemsize 4
>> range start 10955980800 end 10955984896 length 4096
>> ....
>> file tree key (3038 ROOT_ITEM 80009)
>> ...
>> leaf 128958464 items 37 free space 6032 generation 62656 owner FS_TREE
>> ...
>> item 1 key (997645 INODE_ITEM 0) itemoff 15220 itemsize 160
>> generation 62656 transid 62656 size 4943 nbytes 8192
>> block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
>> sequence 5 flags 0x0(none)
>> atime 1478246492.0 (2016-11-04 08:01:32)
>> ctime 1478246493.129060242 (2016-11-04 08:01:33)
>> mtime 1477487995.0 (2016-10-26 13:19:55)
>> otime 1478246493.129060242 (2016-11-04 08:01:33)
>> item 2 key (997645 INODE_REF 299949) itemoff 15200 itemsize 20
>> index 13 namelen 10 name: as_map.hpp
>> item 3 key (997645 EXTENT_DATA 0) itemoff 15147 itemsize 53
>> generation 62656 type 1 (regular)
>> extent data disk byte 10955980800 nr 4096
>> extent data offset 0 nr 8192 ram 8192
>> extent compression 2 (lzo)
>> ...
>>
>> =====[ CAUSE ]=====
>>
>> In main's first call to map_one_extent(10955980800, 4096, 0):
>>
>> * btrfs_search_slot() looks for (10955980800, 0, 0), and:
>> ** returns 1 because it doesn't exist
>> ** sets path->slots[0] to 203 (for leaf 316456960), where it should go if
>> inserted (pointing after the last existing item)
>> * ret is reset to 0
>> * 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.
>> * With (!search_forward && key.objectid > logical) being false, the code calling
>> btrfs_next_item() is not run.
>> * logical is set to this too-low key.objectid
>> * !ret, so set *logical_ret and *len_ret with the new values
>>
>> Back in main:
>>
>> * ret is 0, so don't print the first error
>> * ret is still 0, so don't run map_one_extent() with search_forward=1
>> * At the while loop, (10955960320 + 4096 >= 10955980800 && 10955960320 <
>> 10955980800 + 4096) (10955964416 >= 10955980800 && 10955960320 < 10955984896)
>> (false && true) (false), so don't call map_one_extent() with search_forward=1
>> here, either.
>> * In the while loop, now call map_one_extent() with search_forward=1
>> * !found, so print (somewhat deceptive) error only mentioning the user-given
>> logical without mentioning it looked elsewhere, and give up.
>>
>> =====[ FIX ]=====
>>
>> btrfs-map-logical and I are not friends. The "least code" fix for this
>> situation is this patch.
>>
>> Qu's "btrfs-progs: check: Also compare data between mirrors to detect corruption
>> for NODATASUM extents" uses a simpler way, which makes me wonder if that should
>> just be modified to replace how btrfs-map-logical works. But, I'll admit I do
>> not have my head around the entire way everything is done in btrfs-map-logical,
>> and there could be reasons for it needing to be different here.
>>
>> This doesn't touch what I think is the buffer over-read error described above.
>>
>> With this fix, btrfs_item_key_to_cpu() is not asked to read out of bounds, so
>> map_one_extent() leaves cur_logical and cur_len unmodified because they don't
>> need to be. (Both the first time it's ran with search_forward=0, and in the
>> while loop when it's ran with search_forward=1.)
>>
>> Also updated call from btrfs_next_item() to btrfs_next_extent_item(). I can't
>> see any reason not to, while this area's being modified. It looks for both
>> BTRFS_EXTENT_ITEM_KEY and BTRFS_METADATA_ITEM_KEY, which is what we need.
>> (Granted, inside, it's just calling btrfs_next_item().)
>>
> Make sense. IMP the commit message is too long to read.
> Code wise is almost fine. Some nitpicks are below.
>
>> Also fixed misspelling of "foward" to "forward".
>>
>> Signed-off-by: James Harvey <jamespharvey20@xxxxxxxxx>
>> ---
>> btrfs-map-logical.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/btrfs-map-logical.c b/btrfs-map-logical.c
>> index 7a8bcff9..1c30b22b 100644
>> --- a/btrfs-map-logical.c
>> +++ b/btrfs-map-logical.c
>> @@ -39,7 +39,7 @@
>> static FILE *info_file;
>>
>> static int map_one_extent(struct btrfs_fs_info *fs_info,
>> - u64 *logical_ret, u64 *len_ret, int search_foward)
>> + u64 *logical_ret, u64 *len_ret, int search_forward)
>> {
>> struct btrfs_path *path;
>> struct btrfs_key key;
>> @@ -65,17 +65,25 @@ static int map_one_extent(struct btrfs_fs_info *fs_info,
>> BUG_ON(ret == 0);
>> ret = 0;
>>
>> + if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
>> + ret = btrfs_next_leaf(fs_info->extent_root, path);
>> + if (ret != 0) {
>> + ret = -1;
>
> btrfs_next_leaf() may return -EIO, so keep the negative return code is
> prefered.
> btrfs_next_leaf may return > 0, here I'd like to set ret=-ENOENT.
> You can refer callers of btrfs_next_leaf() how to handle the return codes.
>
>> + goto out;
>> + }
>> + }
>> +
>> again:
>> btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> - if ((search_foward && key.objectid < logical) ||
>> - (!search_foward && key.objectid > logical) ||
>> + if ((search_forward && key.objectid < logical) ||
>> + (!search_forward && key.objectid > logical) ||
>> (key.type != BTRFS_EXTENT_ITEM_KEY &&
>> key.type != BTRFS_METADATA_ITEM_KEY)) {
>> - if (!search_foward)
>> + if (!search_forward)
>> ret = btrfs_previous_extent_item(fs_info->extent_root,
>> path, 0);
>> else
>> - ret = btrfs_next_item(fs_info->extent_root, path);
>> + ret =
> Nit:
> Misclick here?
That would be your (and my) mailer's fault.
If there is a real change line, it will be a leading '+'.
Thanks,
Qu
>
> Thanks,
> Su
>
>> btrfs_next_extent_item(fs_info->extent_root, path);
>> if (ret)
>> goto out;
>> goto again;
>> --
>> 2.17.0
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
