diff mbox series

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

Message ID 20250207014809.1573841-2-andrii@kernel.org (mailing list archive)
State Accepted
Commit 4eb93fea59199c1aa6a6f837e90bdd597804cdcc
Delegated to: BPF
Headers show
Series [bpf-next,1/2] libbpf: fix LDX/STX/ST CO-RE relocation size adjustment logic | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers warning 12 maintainers not CCed: martin.lau@linux.dev kpsingh@kernel.org sdf@fomichev.me jolsa@kernel.org yonghong.song@linux.dev song@kernel.org shuah@kernel.org john.fastabend@gmail.com linux-kselftest@vger.kernel.org haoluo@google.com mykolal@fb.com eddyz87@gmail.com
netdev/build_clang success Errors and warnings before: 47 this patch: 47
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 10 this patch: 10
netdev/checkpatch warning WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-1 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-3 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for x86_64-gcc / build-release

Commit Message

Andrii Nakryiko Feb. 7, 2025, 1:48 a.m. UTC 
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

Comments

Cupertino Miranda Feb. 10, 2025, 8:12 p.m. UTC | #1 
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;
>   }
>
Andrii Nakryiko Feb. 11, 2025, 12:33 a.m. UTC | #2 
On Mon, Feb 10, 2025 at 12:13 PM Cupertino Miranda
<cupertino.miranda@oracle.com> wrote:
>
> 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 ?
>

Well, I only care to get LDX instruction with associated in->a CO-RE
relocation. This is what Clang currently generates for this piece of
code. You can see that it combines both LDX+CO-RE relo for a[0], and
then non-CO-RE relocated LDX for a[1], a[2], a[3], where the base was
relocated with CO-RE a bit earlier.

      44:       18 07 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r7 = 0x0 ll
                0000000000000160:  R_BPF_64_64  data

...

      55:       b7 01 00 00 00 00 00 00 r1 = 0x0
                00000000000001b8:  CO-RE <byte_off> [5] struct
core_reloc_arrays::a (0:0)
      56:       18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x0 ll
                00000000000001c0:  R_BPF_64_64  data
      58:       0f 12 00 00 00 00 00 00 r2 += r1
      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)
      60:       61 23 04 00 00 00 00 00 w3 = *(u32 *)(r2 + 0x4)
      61:       0c 13 00 00 00 00 00 00 w3 += w1
      62:       61 21 08 00 00 00 00 00 w1 = *(u32 *)(r2 + 0x8)
      63:       0c 13 00 00 00 00 00 00 w3 += w1
      64:       61 21 0c 00 00 00 00 00 w1 = *(u32 *)(r2 + 0xc)
      65:       0c 13 00 00 00 00 00 00 w3 += w1
      66:       63 37 04 01 00 00 00 00 *(u32 *)(r7 + 0x104) = w3

Clang might change code generation pattern in the future, of course,
but at least as of right now I know I did test this logic :) Ideally
I'd be able to generate embedded asm with CO-RE relocation, but I'm
not sure that's supported today.

> > +
> >       return 0;
> >   }
> >
>
Cupertino Miranda Feb. 11, 2025, 10:27 a.m. UTC | #3 
On 11-02-2025 00:33, Andrii Nakryiko wrote:
> On Mon, Feb 10, 2025 at 12:13 PM Cupertino Miranda
> <cupertino.miranda@oracle.com> wrote:
>>
>> 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 ?
>>
> 
> Well, I only care to get LDX instruction with associated in->a CO-RE
> relocation. This is what Clang currently generates for this piece of
> code. You can see that it combines both LDX+CO-RE relo for a[0], and
> then non-CO-RE relocated LDX for a[1], a[2], a[3], where the base was
> relocated with CO-RE a bit earlier.
> 
>        44:       18 07 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r7 = 0x0 ll
>                  0000000000000160:  R_BPF_64_64  data
> 
> ...
> 
>        55:       b7 01 00 00 00 00 00 00 r1 = 0x0
>                  00000000000001b8:  CO-RE <byte_off> [5] struct
> core_reloc_arrays::a (0:0)
>        56:       18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x0 ll
>                  00000000000001c0:  R_BPF_64_64  data
>        58:       0f 12 00 00 00 00 00 00 r2 += r1
>        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)
>        60:       61 23 04 00 00 00 00 00 w3 = *(u32 *)(r2 + 0x4)
>        61:       0c 13 00 00 00 00 00 00 w3 += w1
>        62:       61 21 08 00 00 00 00 00 w1 = *(u32 *)(r2 + 0x8)
>        63:       0c 13 00 00 00 00 00 00 w3 += w1
>        64:       61 21 0c 00 00 00 00 00 w1 = *(u32 *)(r2 + 0xc)
>        65:       0c 13 00 00 00 00 00 00 w3 += w1
>        66:       63 37 04 01 00 00 00 00 *(u32 *)(r7 + 0x104) = w3
> 
> Clang might change code generation pattern in the future, of course,
> but at least as of right now I know I did test this logic :) Ideally
> I'd be able to generate embedded asm with CO-RE relocation, but I'm
> not sure that's supported today.
Ok, good! I just miss read it. :)
Thanks!
> 
>>> +
>>>        return 0;
>>>    }
>>>
>>
diff mbox series

Patch

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];
+
 	return 0;
 }