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

Cupertino Miranda <cupertino.miranda@xxxxxxxxxx> · Tue, 11 Feb 2025 10:27:13 +0000

On 11-02-2025 00:33, Andrii Nakryiko wrote:
On Mon, Feb 10, 2025 at 12:13 PM Cupertino Miranda
<cupertino.miranda@xxxxxxxxxx> 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@xxxxxxxxxx>
---
   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;
   }










