Re: [PATCH bpf-next 1/2] libbpf: fix LDX/STX/ST CO-RE relocation size adjustment logic

[Eduard Zingerman <eddyz87@gmail.com>]

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@etsalapatis.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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@gmail.com>?

>  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.

[...]