On Thu, 2025-02-06 at 17:48 -0800, Andrii Nakryiko wrote:
> Libbpf has a somewhat obscure feature of automatically adjusting the
> "size" of LDX/STX/ST instruction (memory store and load instructions),
> based on originally recorded access size (u8, u16, u32, or u64) and the
> actual size of the field on target kernel. This is meant to facilitate
> using BPF CO-RE on 32-bit architectures (pointers are always 64-bit in
> BPF, but host kernel's BTF will have it as 32-bit type), as well as
> generally supporting safe type changes (unsigned integer type changes
> can be transparently "relocated").
>
> One issue that surfaced only now, 5 years after this logic was
> implemented, is how this all works when dealing with fields that are
> arrays. This isn't all that easy and straightforward to hit (see
> selftests that reproduce this condition), but one of sched_ext BPF
> programs did hit it with innocent looking loop.
>
> Long story short, libbpf used to calculate entire array size, instead of
> making sure to only calculate array's element size. But it's the element
> that is loaded by LDX/STX/ST instructions (1, 2, 4, or 8 bytes), so
> that's what libbpf should check. This patch adjusts the logic for
> arrays and fixed the issue.
>
> Reported-by: Emil Tsalapatis <emil@xxxxxxxxxxxxxxx>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
Do I understand correctly, that for nested arrays relocation size
would be resolved to the innermost element size?
To allow e.g.:
struct { int a[2][3]; }
...
int *a = __builtin_preserve_access_index(({ in->a; }));
a[0] = 42;
With a justification that nothing useful could be done with 'int **a'
type when dimensions are not known?
I guess this makes sense.
Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>?
> tools/lib/bpf/relo_core.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> index 7632e9d41827..2b83c98a1137 100644
> --- a/tools/lib/bpf/relo_core.c
> +++ b/tools/lib/bpf/relo_core.c
> @@ -683,7 +683,7 @@ static int bpf_core_calc_field_relo(const char *prog_name,
> {
> const struct bpf_core_accessor *acc;
> const struct btf_type *t;
> - __u32 byte_off, byte_sz, bit_off, bit_sz, field_type_id;
> + __u32 byte_off, byte_sz, bit_off, bit_sz, field_type_id, elem_id;
> const struct btf_member *m;
> const struct btf_type *mt;
> bool bitfield;
> @@ -706,8 +706,14 @@ static int bpf_core_calc_field_relo(const char *prog_name,
> if (!acc->name) {
> if (relo->kind == BPF_CORE_FIELD_BYTE_OFFSET) {
> *val = spec->bit_offset / 8;
> - /* remember field size for load/store mem size */
> - sz = btf__resolve_size(spec->btf, acc->type_id);
> + /* remember field size for load/store mem size;
> + * note, for arrays we care about individual element
> + * sizes, not the overall array size
> + */
> + t = skip_mods_and_typedefs(spec->btf, acc->type_id, &elem_id);
> + while (btf_is_array(t))
> + t = skip_mods_and_typedefs(spec->btf, btf_array(t)->type, &elem_id);
> + sz = btf__resolve_size(spec->btf, elem_id);
Nit: while trying to figure out what this change is about
I commented out the above hunk and this did not trigger any test failures.
[...]
