| Message ID | 20190503003054.9346-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: tree-checker: Check if the file extent end overflow | expand |
On Fri, May 03, 2019 at 08:30:54AM +0800, Qu Wenruo wrote: > Under certain condition, we could have strange file extent item in log > tree like: > > item 18 key (69599 108 397312) itemoff 15208 itemsize 53 > extent data disk bytenr 0 nr 0 > extent data offset 0 nr 18446744073709547520 ram 18446744073709547520 > > The num_bytes and ram_bytes part overflow. > > For num_bytes part, we can detect such overflow along with file offset > (key->offset), as file_offset + num_bytes should never go beyond u64 > max. > > For ram_bytes part, it's about the decompressed size of the extent, not > directly related to the size. > In theory is OK to have super large value, and put extra > limitation on ram bytes may cause unexpected false alert. > > So in tree-checker, we only check if the file offset and num bytes > overflow. > > Signed-off-by: Qu Wenruo <wqu@suse.com> So this patch can be dropped because of "Btrfs: tree-checker: detect file extent items with overlapping ranges", right?
On 2019/5/16 下午8:57, David Sterba wrote: > On Fri, May 03, 2019 at 08:30:54AM +0800, Qu Wenruo wrote: >> Under certain condition, we could have strange file extent item in log >> tree like: >> >> item 18 key (69599 108 397312) itemoff 15208 itemsize 53 >> extent data disk bytenr 0 nr 0 >> extent data offset 0 nr 18446744073709547520 ram 18446744073709547520 >> >> The num_bytes and ram_bytes part overflow. >> >> For num_bytes part, we can detect such overflow along with file offset >> (key->offset), as file_offset + num_bytes should never go beyond u64 >> max. >> >> For ram_bytes part, it's about the decompressed size of the extent, not >> directly related to the size. >> In theory is OK to have super large value, and put extra >> limitation on ram bytes may cause unexpected false alert. >> >> So in tree-checker, we only check if the file offset and num bytes >> overflow. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > So this patch can be dropped because of "Btrfs: tree-checker: detect > file extent items with overlapping ranges", right? > Nope, there is still a case can be detected by this patch only. If the last file extent overflow, that patch can not detect it. Although that patch has a much better coverage than this one. Thanks, Qu
On Fri, May 03, 2019 at 08:30:54AM +0800, Qu Wenruo wrote: > Under certain condition, we could have strange file extent item in log > tree like: > > item 18 key (69599 108 397312) itemoff 15208 itemsize 53 > extent data disk bytenr 0 nr 0 > extent data offset 0 nr 18446744073709547520 ram 18446744073709547520 > > The num_bytes and ram_bytes part overflow. > > For num_bytes part, we can detect such overflow along with file offset > (key->offset), as file_offset + num_bytes should never go beyond u64 > max. > > For ram_bytes part, it's about the decompressed size of the extent, not > directly related to the size. > In theory is OK to have super large value, and put extra > limitation on ram bytes may cause unexpected false alert. > > So in tree-checker, we only check if the file offset and num bytes > overflow. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 4f4f5af6345a..795eb6c18456 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -112,6 +112,7 @@ static int check_extent_data_item(struct extent_buffer *leaf, > struct btrfs_file_extent_item *fi; > u32 sectorsize = fs_info->sectorsize; > u32 item_size = btrfs_item_size_nr(leaf, slot); > + u64 extent_end; > > if (!IS_ALIGNED(key->offset, sectorsize)) { > file_extent_err(leaf, slot, > @@ -186,6 +187,14 @@ static int check_extent_data_item(struct extent_buffer *leaf, > CHECK_FE_ALIGNED(leaf, slot, fi, offset, sectorsize) || > CHECK_FE_ALIGNED(leaf, slot, fi, num_bytes, sectorsize)) > return -EUCLEAN; > + /* Catch extent end overflow case */ > + if (check_add_overflow(btrfs_file_extent_num_bytes(leaf, fi), > + key->offset, &extent_end)) { > + file_extent_err(leaf, slot, > + "extent end overflow, have file offset %llu extent num bytes %llu", > + key->offset, > + btrfs_file_extent_num_bytes(leaf, fi)); Isn't this missing return -EUCLEAN; ? > + } > return 0; > } > > -- > 2.21.0
On 2019/5/30 下午8:02, David Sterba wrote: > On Fri, May 03, 2019 at 08:30:54AM +0800, Qu Wenruo wrote: >> Under certain condition, we could have strange file extent item in log >> tree like: >> >> item 18 key (69599 108 397312) itemoff 15208 itemsize 53 >> extent data disk bytenr 0 nr 0 >> extent data offset 0 nr 18446744073709547520 ram 18446744073709547520 >> >> The num_bytes and ram_bytes part overflow. >> >> For num_bytes part, we can detect such overflow along with file offset >> (key->offset), as file_offset + num_bytes should never go beyond u64 >> max. >> >> For ram_bytes part, it's about the decompressed size of the extent, not >> directly related to the size. >> In theory is OK to have super large value, and put extra >> limitation on ram bytes may cause unexpected false alert. >> >> So in tree-checker, we only check if the file offset and num bytes >> overflow. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/tree-checker.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >> index 4f4f5af6345a..795eb6c18456 100644 >> --- a/fs/btrfs/tree-checker.c >> +++ b/fs/btrfs/tree-checker.c >> @@ -112,6 +112,7 @@ static int check_extent_data_item(struct extent_buffer *leaf, >> struct btrfs_file_extent_item *fi; >> u32 sectorsize = fs_info->sectorsize; >> u32 item_size = btrfs_item_size_nr(leaf, slot); >> + u64 extent_end; >> >> if (!IS_ALIGNED(key->offset, sectorsize)) { >> file_extent_err(leaf, slot, >> @@ -186,6 +187,14 @@ static int check_extent_data_item(struct extent_buffer *leaf, >> CHECK_FE_ALIGNED(leaf, slot, fi, offset, sectorsize) || >> CHECK_FE_ALIGNED(leaf, slot, fi, num_bytes, sectorsize)) >> return -EUCLEAN; >> + /* Catch extent end overflow case */ >> + if (check_add_overflow(btrfs_file_extent_num_bytes(leaf, fi), >> + key->offset, &extent_end)) { >> + file_extent_err(leaf, slot, >> + "extent end overflow, have file offset %llu extent num bytes %llu", >> + key->offset, >> + btrfs_file_extent_num_bytes(leaf, fi)); > > Isn't this missing > > return -EUCLEAN; Oh, my bad. Would you mind to fold that return line? Thanks, Qu > > ? > >> + } >> return 0; >> } >> >> -- >> 2.21.0
On Thu, May 30, 2019 at 08:03:48PM +0800, Qu Wenruo wrote: > >> @@ -112,6 +112,7 @@ static int check_extent_data_item(struct extent_buffer *leaf, > >> struct btrfs_file_extent_item *fi; > >> u32 sectorsize = fs_info->sectorsize; > >> u32 item_size = btrfs_item_size_nr(leaf, slot); > >> + u64 extent_end; > >> > >> if (!IS_ALIGNED(key->offset, sectorsize)) { > >> file_extent_err(leaf, slot, > >> @@ -186,6 +187,14 @@ static int check_extent_data_item(struct extent_buffer *leaf, > >> CHECK_FE_ALIGNED(leaf, slot, fi, offset, sectorsize) || > >> CHECK_FE_ALIGNED(leaf, slot, fi, num_bytes, sectorsize)) > >> return -EUCLEAN; > >> + /* Catch extent end overflow case */ > >> + if (check_add_overflow(btrfs_file_extent_num_bytes(leaf, fi), > >> + key->offset, &extent_end)) { > >> + file_extent_err(leaf, slot, > >> + "extent end overflow, have file offset %llu extent num bytes %llu", > >> + key->offset, > >> + btrfs_file_extent_num_bytes(leaf, fi)); > > > > Isn't this missing > > > > return -EUCLEAN; > > Oh, my bad. > > Would you mind to fold that return line? Udated and added to misc-next.
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 4f4f5af6345a..795eb6c18456 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -112,6 +112,7 @@ static int check_extent_data_item(struct extent_buffer *leaf, struct btrfs_file_extent_item *fi; u32 sectorsize = fs_info->sectorsize; u32 item_size = btrfs_item_size_nr(leaf, slot); + u64 extent_end; if (!IS_ALIGNED(key->offset, sectorsize)) { file_extent_err(leaf, slot, @@ -186,6 +187,14 @@ static int check_extent_data_item(struct extent_buffer *leaf, CHECK_FE_ALIGNED(leaf, slot, fi, offset, sectorsize) || CHECK_FE_ALIGNED(leaf, slot, fi, num_bytes, sectorsize)) return -EUCLEAN; + /* Catch extent end overflow case */ + if (check_add_overflow(btrfs_file_extent_num_bytes(leaf, fi), + key->offset, &extent_end)) { + file_extent_err(leaf, slot, + "extent end overflow, have file offset %llu extent num bytes %llu", + key->offset, + btrfs_file_extent_num_bytes(leaf, fi)); + } return 0; }
Under certain condition, we could have strange file extent item in log tree like: item 18 key (69599 108 397312) itemoff 15208 itemsize 53 extent data disk bytenr 0 nr 0 extent data offset 0 nr 18446744073709547520 ram 18446744073709547520 The num_bytes and ram_bytes part overflow. For num_bytes part, we can detect such overflow along with file offset (key->offset), as file_offset + num_bytes should never go beyond u64 max. For ram_bytes part, it's about the decompressed size of the extent, not directly related to the size. In theory is OK to have super large value, and put extra limitation on ram bytes may cause unexpected false alert. So in tree-checker, we only check if the file offset and num bytes overflow. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tree-checker.c | 9 +++++++++ 1 file changed, 9 insertions(+)