Re: BTF CO-RE bitfield relocation: why is the load size rounded?

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Oct 2, 2020 at 4:25 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Hi Andrii,
>
> I'm trying to understand the following code from
> bpf_core_calc_field_relo in libbpf.c:
>
>     if (bitfield) {
>         byte_sz = mt->size;
>         byte_off = bit_off / 8 / byte_sz * byte_sz;
>         /* figure out smallest int size necessary for bitfield load */
>         while (bit_off + bit_sz - byte_off * 8 > byte_sz * 8) {
>             if (byte_sz >= 8) {
>                 ...
>                 return -E2BIG;
>             }
>             byte_sz *= 2;
>             byte_off = bit_off / 8 / byte_sz * byte_sz;
>         }
>     }
>
> It's used to calculate the load size (byte_sz) and load offset
> (byte_off) for a bitfield member of a struct. Can you explain to me
> why byte_off is rounded to byte_sz? Is it to preserve alignment?
>
> It seems like the rounding can lead to reading past the end of a
> struct. An example:
>
>     struct foo {
>         unsigned short boo:4;
>     } __attribute__((packed));
>
>     [2] STRUCT 'foo' size=1 vlen=1
>         'boo' type_id=3 bits_offset=0 bitfield_size=4
>     [3] INT 'unsigned short' size=2 bits_offset=0 nr_bits=16 encoding=(none)
>
> The result of the calculation for this is byte_sz = 2 and byte_off =
> 0, but the structure is only 1 byte in length.
>
> Would it be possible to replace the calculation with the following?
>
>     byte_off = bit_off / 8
>     byte_sz = ((bit_off + bit_sz + 7) - byte_off*8) / 8

Yes, it's for alignment. See BPF_CORE_READ_BITFIELD(), which reads the
underlying integer directly, which means that it has to be a
naturally-aligned memory read. Yes, it technically could be a problem
that we'll read 2 bytes in this case, but I don't think the kernel is
using that many packed structs and bitfields of interest are in the
very last byte of a packed struct... Packed structs are special and
annoying, but luckily not all that popular.

In this particular case reading one byte would be enough, so we could
change the logic to always start with a byte and see what's the
smallest int type we can get away with, ignoring what's the underlying
integer type (unsigned short in this case) was declared. But I'm not
sure that's a real problem worth solving.

But first, do you see any real issues in production with the current logic?

>
> Thanks
> Lorenz
>
> --
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com