Message ID | 20200812060509.71590-2-wqu@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs: Enhanced runtime defence against fuzzed images | expand |
On Wed, Aug 12, 2020 at 02:05:06PM +0800, Qu Wenruo wrote: > +/* > + * Check if the [start, start + len) range is valid before reading/writing > + * the eb. > + * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address. > + * > + * Caller should not touch the dst/src memory if this function returns error. > + */ > +static int check_eb_range(const struct extent_buffer *eb, unsigned long start, > + unsigned long len) > +{ > + /* start, start + len should not go beyond eb->len nor overflow */ > + if (unlikely(start > eb->len || start + len > eb->len || > + len > eb->len)) { > + btrfs_warn(eb->fs_info, > +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n", > + eb->start, eb->len, start, len); > + WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)); > + return -EINVAL; > + } > + return 0; > +} This helper is similar to the check_setget_bounds that have some performance impact, https://lore.kernel.org/linux-btrfs/20200730110943.GE3703@twin.jikos.cz/ . The extent buffer helpers are not called that often as the setget helpers but still it could be improved to avoid the function call penalty on the hot path. static inline in check_eb_range(...) { if (unlikely(out of range)) return report_eb_range(...) return 0; } In the original code the range check was open coded and the above will lead to the same asm output, while keeping the C code readable.
On 2020/8/13 下午10:05, David Sterba wrote: > On Wed, Aug 12, 2020 at 02:05:06PM +0800, Qu Wenruo wrote: >> +/* >> + * Check if the [start, start + len) range is valid before reading/writing >> + * the eb. >> + * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address. >> + * >> + * Caller should not touch the dst/src memory if this function returns error. >> + */ >> +static int check_eb_range(const struct extent_buffer *eb, unsigned long start, >> + unsigned long len) >> +{ >> + /* start, start + len should not go beyond eb->len nor overflow */ >> + if (unlikely(start > eb->len || start + len > eb->len || >> + len > eb->len)) { >> + btrfs_warn(eb->fs_info, >> +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n", >> + eb->start, eb->len, start, len); >> + WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)); >> + return -EINVAL; >> + } >> + return 0; >> +} > > This helper is similar to the check_setget_bounds that have some > performance impact, > https://lore.kernel.org/linux-btrfs/20200730110943.GE3703@twin.jikos.cz/ > . > > The extent buffer helpers are not called that often as the setget > helpers but still it could be improved to avoid the function call > penalty on the hot path. > > static inline in check_eb_range(...) { > if (unlikely(out of range)) > return report_eb_range(...) > return 0; > } I thought we should avoid manual inline, but let the compiler to determine if it's needed. Or in this particular case, we're better than the compiler? Thanks, Qu > > In the original code the range check was open coded and the above will > lead to the same asm output, while keeping the C code readable. >
On Fri, Aug 14, 2020 at 08:47:17AM +0800, Qu Wenruo wrote: > > > On 2020/8/13 下午10:05, David Sterba wrote: > > On Wed, Aug 12, 2020 at 02:05:06PM +0800, Qu Wenruo wrote: > >> +/* > >> + * Check if the [start, start + len) range is valid before reading/writing > >> + * the eb. > >> + * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address. > >> + * > >> + * Caller should not touch the dst/src memory if this function returns error. > >> + */ > >> +static int check_eb_range(const struct extent_buffer *eb, unsigned long start, > >> + unsigned long len) > >> +{ > >> + /* start, start + len should not go beyond eb->len nor overflow */ > >> + if (unlikely(start > eb->len || start + len > eb->len || > >> + len > eb->len)) { > >> + btrfs_warn(eb->fs_info, > >> +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n", > >> + eb->start, eb->len, start, len); > >> + WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)); > >> + return -EINVAL; > >> + } > >> + return 0; > >> +} > > > > This helper is similar to the check_setget_bounds that have some > > performance impact, > > https://lore.kernel.org/linux-btrfs/20200730110943.GE3703@twin.jikos.cz/ > > . > > > > The extent buffer helpers are not called that often as the setget > > helpers but still it could be improved to avoid the function call > > penalty on the hot path. > > > > static inline in check_eb_range(...) { > > if (unlikely(out of range)) > > return report_eb_range(...) > > return 0; > > } > > I thought we should avoid manual inline, but let the compiler to > determine if it's needed. > > Or in this particular case, we're better than the compiler? In general it shouldn't be necessary to inline or partition the functions. In the check_setget case it had a noticeable impact on performance, so crafting the hot path manually produces a better assembly and does not depend on the compiler optimizations. The important part here is that this has been analyzed and measured that it really makes a difference. For sanity checks I think we should try to make it as fast as possible, it's better to have them then not but we also don't want to sacrifice performance. I haven't analyzed the asm code impact in this patch but the pattern and flow of check_eb_range is the same.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 617ea38e6fd7..9f583ef1e387 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -5620,6 +5620,28 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num) return ret; } +/* + * Check if the [start, start + len) range is valid before reading/writing + * the eb. + * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address. + * + * Caller should not touch the dst/src memory if this function returns error. + */ +static int check_eb_range(const struct extent_buffer *eb, unsigned long start, + unsigned long len) +{ + /* start, start + len should not go beyond eb->len nor overflow */ + if (unlikely(start > eb->len || start + len > eb->len || + len > eb->len)) { + btrfs_warn(eb->fs_info, +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n", + eb->start, eb->len, start, len); + WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)); + return -EINVAL; + } + return 0; +} + void read_extent_buffer(const struct extent_buffer *eb, void *dstv, unsigned long start, unsigned long len) { @@ -5630,12 +5652,8 @@ void read_extent_buffer(const struct extent_buffer *eb, void *dstv, char *dst = (char *)dstv; unsigned long i = start >> PAGE_SHIFT; - if (start + len > eb->len) { - WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n", - eb->start, eb->len, start, len); - memset(dst, 0, len); + if (check_eb_range(eb, start, len)) return; - } offset = offset_in_page(start); @@ -5700,8 +5718,8 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv, unsigned long i = start >> PAGE_SHIFT; int ret = 0; - WARN_ON(start > eb->len); - WARN_ON(start + len > eb->start + eb->len); + if (check_eb_range(eb, start, len)) + return -EINVAL; offset = offset_in_page(start); @@ -5754,8 +5772,8 @@ void write_extent_buffer(const struct extent_buffer *eb, const void *srcv, char *src = (char *)srcv; unsigned long i = start >> PAGE_SHIFT; - WARN_ON(start > eb->len); - WARN_ON(start + len > eb->start + eb->len); + if (check_eb_range(eb, start, len)) + return; offset = offset_in_page(start); @@ -5783,8 +5801,8 @@ void memzero_extent_buffer(const struct extent_buffer *eb, unsigned long start, char *kaddr; unsigned long i = start >> PAGE_SHIFT; - WARN_ON(start > eb->len); - WARN_ON(start + len > eb->start + eb->len); + if (check_eb_range(eb, start, len)) + return; offset = offset_in_page(start); @@ -5828,6 +5846,10 @@ void copy_extent_buffer(const struct extent_buffer *dst, char *kaddr; unsigned long i = dst_offset >> PAGE_SHIFT; + if (check_eb_range(dst, dst_offset, len) || + check_eb_range(src, src_offset, len)) + return; + WARN_ON(src->len != dst_len); offset = offset_in_page(dst_offset); @@ -6017,25 +6039,15 @@ void memcpy_extent_buffer(const struct extent_buffer *dst, unsigned long dst_offset, unsigned long src_offset, unsigned long len) { - struct btrfs_fs_info *fs_info = dst->fs_info; size_t cur; size_t dst_off_in_page; size_t src_off_in_page; unsigned long dst_i; unsigned long src_i; - if (src_offset + len > dst->len) { - btrfs_err(fs_info, - "memmove bogus src_offset %lu move len %lu dst len %lu", - src_offset, len, dst->len); - BUG(); - } - if (dst_offset + len > dst->len) { - btrfs_err(fs_info, - "memmove bogus dst_offset %lu move len %lu dst len %lu", - dst_offset, len, dst->len); - BUG(); - } + if (check_eb_range(dst, dst_offset, len) || + check_eb_range(dst, src_offset, len)) + return; while (len > 0) { dst_off_in_page = offset_in_page(dst_offset); @@ -6062,7 +6074,6 @@ void memmove_extent_buffer(const struct extent_buffer *dst, unsigned long dst_offset, unsigned long src_offset, unsigned long len) { - struct btrfs_fs_info *fs_info = dst->fs_info; size_t cur; size_t dst_off_in_page; size_t src_off_in_page; @@ -6071,18 +6082,9 @@ void memmove_extent_buffer(const struct extent_buffer *dst, unsigned long dst_i; unsigned long src_i; - if (src_offset + len > dst->len) { - btrfs_err(fs_info, - "memmove bogus src_offset %lu move len %lu len %lu", - src_offset, len, dst->len); - BUG(); - } - if (dst_offset + len > dst->len) { - btrfs_err(fs_info, - "memmove bogus dst_offset %lu move len %lu len %lu", - dst_offset, len, dst->len); - BUG(); - } + if (check_eb_range(dst, dst_offset, len) || + check_eb_range(dst, src_offset, len)) + return; if (dst_offset < src_offset) { memcpy_extent_buffer(dst, dst_offset, src_offset, len); return;