Re: [PATCH 3/7] btrfs: accessors: inline eb bounds check and factor out the error report

Boris Burkov
2025-07-02T17:59:21Z

On Tue, Jul 01, 2025 at 07:23:50PM +0200, David Sterba wrote:
> There's a check in each set/get helper if the requested range is within
> extent buffer bounds, and if it's not then report it. This was in an
> ASSERT statement so with CONFIG_BTRFS_ASSERT this crashes right away, on
> other configs this is only reported but reading out of the bounds is
> done anyway. There are currently no known reports of this particular
> condition failing.
>
> There are some drawbacks though. The behaviour dependence on the
> assertions being compiled in or not and a less visible effect of
> inlining report_setget_bounds() into each helper.
>
> As the bounds check is expected to succeed almost always it's ok to
> inline it but make the report a function and move it out of the helper
> completely (__cold puts it to a different section). This also skips
> reading/writing the requested range in case it fails.
>
> This improves stack usage significantly:
>
> btrfs_get_16 -48 (80 -> 32)
> btrfs_get_32 -48 (80 -> 32)
> btrfs_get_64 -48 (80 -> 32)
> btrfs_get_8 -48 (72 -> 24)
> btrfs_set_16 -56 (88 -> 32)
> btrfs_set_32 -56 (88 -> 32)
> btrfs_set_64 -56 (88 -> 32)
> btrfs_set_8 -48 (80 -> 32)
>
> NEW (48):
> report_setget_bounds 48
> LOST/NEW DELTA: +48
> PRE/POST DELTA: -360
>
> Same as .ko size:
>
> text data bss dec hex filename
> 1456079 115665 16088 1587832 183a78 pre/btrfs.ko
> 1454951 115665 16088 1586704 183610 post/btrfs.ko
>
> DELTA: -1128
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/accessors.c | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/fs/btrfs/accessors.c b/fs/btrfs/accessors.c
> index 2e90b9b14e73f4..a7b6b2d7bde224 100644
> --- a/fs/btrfs/accessors.c
> +++ b/fs/btrfs/accessors.c
> @@ -9,20 +9,15 @@
> #include "fs.h"
> #include "accessors.h"
>
> -static bool check_setget_bounds(const struct extent_buffer *eb,
> - const void *ptr, unsigned off, int size)
> +static void __cold report_setget_bounds(const struct extent_buffer *eb,
> + const void *ptr, unsigned off, int size)
> {
> - const unsigned long member_offset = (unsigned long)ptr + off;
> + unsigned long member_offset = (unsigned long)ptr + off;
>
> - if (unlikely(member_offset + size > eb->len)) {
> - btrfs_warn(eb->fs_info,
> - "bad eb member %s: ptr 0x%lx start %llu member offset %lu size %d",
> - (member_offset > eb->len ? "start" : "end"),
> - (unsigned long)ptr, eb->start, member_offset, size);
> - return false;
> - }
> -
> - return true;
> + btrfs_warn(eb->fs_info,
> + "bad eb member %s: ptr 0x%lx start %llu member offset %lu size %d",
> + (member_offset > eb->len ? "start" : "end"),
> + (unsigned long)ptr, eb->start, member_offset, size);
> }
>
> /*
> @@ -56,7 +51,10 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb, \
> const int part = eb->folio_size - oil; \
> u8 lebytes[sizeof(u##bits)]; \
> \
> - ASSERT(check_setget_bounds(eb, ptr, off, sizeof(u##bits))); \
> + if (unlikely(member_offset + sizeof(u##bits) > eb->len)) { \
> + report_setget_bounds(eb, ptr, off, sizeof(u##bits)); \

For full no-change compatibility would it make sense to also ASSERT
here? (or stuff it in report, these are the only two users)

> + return 0; \
> + } \
> if (INLINE_EXTENT_BUFFER_PAGES == 1 || likely(sizeof(u##bits) <= part)) \
> return get_unaligned_le##bits(kaddr + oil); \
> \
> @@ -76,7 +74,10 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr, \
> const int part = eb->folio_size - oil; \
> u8 lebytes[sizeof(u##bits)]; \
> \
> - ASSERT(check_setget_bounds(eb, ptr, off, sizeof(u##bits))); \
> + if (unlikely(member_offset + sizeof(u##bits) > eb->len)) { \
> + report_setget_bounds(eb, ptr, off, sizeof(u##bits)); \
> + return; \
> + } \

Would a helper macro to reduce this duplication be useful? Seems
borderline but worth mentioning. Next time you improve it you won't
have to hit two spots.

> if (INLINE_EXTENT_BUFFER_PAGES == 1 || \
> likely(sizeof(u##bits) <= part)) { \
> put_unaligned_le##bits(val, kaddr + oil); \
> --
> 2.49.0
>