Re: [PATCH bpf-next 2/2] selftests/bpf: add test for LDX/STX/ST relocations over array field

[Cupertino Miranda <cupertino.miranda@oracle.com>]

Hi Andrii,

On 07-02-2025 01:48, Andrii Nakryiko wrote:
> Add a simple repro for the issue of miscalculating LDX/STX/ST CO-RE
> relocation size adjustment when the CO-RE relocation target type is an
> ARRAY.
> 
> We need to make sure that compiler generates LDX/STX/ST instruction with
> CO-RE relocation against entire ARRAY type, not ARRAY's element. With
> the code pattern in selftest, we get this:
> 
>        59:       61 71 00 00 00 00 00 00 w1 = *(u32 *)(r7 + 0x0)
>                  00000000000001d8:  CO-RE <byte_off> [5] struct core_reloc_arrays::a (0:0)
> 
> Where offset of `int a[5]` is embedded (through CO-RE relocation) into memory
> load instruction itself.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>   tools/testing/selftests/bpf/prog_tests/core_reloc.c    |  6 ++++--
>   ...f__core_reloc_arrays___err_bad_signed_arr_elem_sz.c |  3 +++
>   tools/testing/selftests/bpf/progs/core_reloc_types.h   | 10 ++++++++++
>   .../selftests/bpf/progs/test_core_reloc_arrays.c       |  5 +++++
>   4 files changed, 22 insertions(+), 2 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_bad_signed_arr_elem_sz.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> index e10ea92c3fe2..08963c82f30b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> @@ -85,11 +85,11 @@ static int duration = 0;
>   #define NESTING_ERR_CASE(name) {					\
>   	NESTING_CASE_COMMON(name),					\
>   	.fails = true,							\
> -	.run_btfgen_fails = true,							\
> +	.run_btfgen_fails = true,					\
>   }
>   
>   #define ARRAYS_DATA(struct_name) STRUCT_TO_CHAR_PTR(struct_name) {	\
> -	.a = { [2] = 1 },						\
> +	.a = { [2] = 1, [3] = 11 },					\
>   	.b = { [1] = { [2] = { [3] = 2 } } },				\
>   	.c = { [1] = { .c =  3 } },					\
>   	.d = { [0] = { [0] = { .d = 4 } } },				\
> @@ -108,6 +108,7 @@ static int duration = 0;
>   	.input_len = sizeof(struct core_reloc_##name),			\
>   	.output = STRUCT_TO_CHAR_PTR(core_reloc_arrays_output) {	\
>   		.a2   = 1,						\
> +		.a3   = 12,						\
>   		.b123 = 2,						\
>   		.c1c  = 3,						\
>   		.d00d = 4,						\
> @@ -602,6 +603,7 @@ static const struct core_reloc_test_case test_cases[] = {
>   	ARRAYS_ERR_CASE(arrays___err_non_array),
>   	ARRAYS_ERR_CASE(arrays___err_wrong_val_type),
>   	ARRAYS_ERR_CASE(arrays___err_bad_zero_sz_arr),
> +	ARRAYS_ERR_CASE(arrays___err_bad_signed_arr_elem_sz),
>   
>   	/* enum/ptr/int handling scenarios */
>   	PRIMITIVES_CASE(primitives),
> diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_bad_signed_arr_elem_sz.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_bad_signed_arr_elem_sz.c
> new file mode 100644
> index 000000000000..21a560427b10
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_bad_signed_arr_elem_sz.c
> @@ -0,0 +1,3 @@
> +#include "core_reloc_types.h"
> +
> +void f(struct core_reloc_arrays___err_bad_signed_arr_elem_sz x) {}
> diff --git a/tools/testing/selftests/bpf/progs/core_reloc_types.h b/tools/testing/selftests/bpf/progs/core_reloc_types.h
> index fd8e1b4c6762..5760ae015e09 100644
> --- a/tools/testing/selftests/bpf/progs/core_reloc_types.h
> +++ b/tools/testing/selftests/bpf/progs/core_reloc_types.h
> @@ -347,6 +347,7 @@ struct core_reloc_nesting___err_too_deep {
>    */
>   struct core_reloc_arrays_output {
>   	int a2;
> +	int a3;
>   	char b123;
>   	int c1c;
>   	int d00d;
> @@ -455,6 +456,15 @@ struct core_reloc_arrays___err_bad_zero_sz_arr {
>   	struct core_reloc_arrays_substruct d[1][2];
>   };
>   
> +struct core_reloc_arrays___err_bad_signed_arr_elem_sz {
> +	/* int -> short (signed!): not supported case */
> +	short a[5];
> +	char b[2][3][4];
> +	struct core_reloc_arrays_substruct c[3];
> +	struct core_reloc_arrays_substruct d[1][2];
> +	struct core_reloc_arrays_substruct f[][2];
> +};
> +
>   /*
>    * PRIMITIVES
>    */
> diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c b/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
> index 51b3f79df523..448403634eea 100644
> --- a/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
> +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
> @@ -15,6 +15,7 @@ struct {
>   
>   struct core_reloc_arrays_output {
>   	int a2;
> +	int a3;
>   	char b123;
>   	int c1c;
>   	int d00d;
> @@ -41,6 +42,7 @@ int test_core_arrays(void *ctx)
>   {
>   	struct core_reloc_arrays *in = (void *)&data.in;
>   	struct core_reloc_arrays_output *out = (void *)&data.out;
> +	int *a;
>   
>   	if (CORE_READ(&out->a2, &in->a[2]))
>   		return 1;
> @@ -53,6 +55,9 @@ int test_core_arrays(void *ctx)
>   	if (CORE_READ(&out->f01c, &in->f[0][1].c))
>   		return 1;
>   
> +	a = __builtin_preserve_access_index(({ in->a; }));
> +	out->a3 = a[0] + a[1] + a[2] + a[3];
Just to try to understand what seems to be the expectation from the 
compiler and CO-RE in this case.
Do you expect that all those a[n] accesses would be generating CO-RE 
relocations assuming the size for the elements in in->a ?

> +
>   	return 0;
>   }
>