Create shadow types for struct_ops maps in skeletons

bpftool is going to reuse this helper function to support shadow types of struct_ops maps.

Signed-off-by Kui-Feng Lee <[email protected]>

bpftool is going to reuse this helper function to support shadow types of struct_ops maps.

Signed-off-by Kui-Feng Lee <[email protected]>

---
 tools/lib/bpf/libbpf.c          | 2 +-
 tools/lib/bpf/libbpf_internal.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +1 -1

index 01f407591a92..ef8fd20f33ca 100644
@@ -2145,7 +2145,7 @@ skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id)
        return t;
 }
 
-static const struct btf_type *
+const struct btf_type *
 resolve_func_ptr(const struct btf *btf, __u32 id, __u32 *res_id)
 {
        const struct btf_type *t;

tools/lib/bpf/libbpf_internal.h +0 -0

index ad936ac5e639..17e6d381da6a 100644

tools/lib/bpf/libbpf_internal.h +0 -1

 static inline enum btf_func_linkage btf_func_linkage(const struct btf_type *t)
 {
--

2.34.1

Quoted 28 lines · Kui-Feng Lee

bpftool is going to reuse this helper function to support shadow types of struct_ops maps.

Signed-off-by Kui-Feng Lee <[email protected]>

---
  tools/lib/bpf/libbpf.c          | 2 +-
  tools/lib/bpf/libbpf_internal.h | 2 ++
  2 files changed, 3 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +1 -1

index 01f407591a92..ef8fd20f33ca 100644
@@ -2145,7 +2145,7 @@ skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id)
         return t;
  }
 
-static const struct btf_type *
+const struct btf_type *
  resolve_func_ptr(const struct btf *btf, __u32 id, __u32 *res_id)
  {
         const struct btf_type *t;

tools/lib/bpf/libbpf_internal.h +0 -0

index ad936ac5e639..17e6d381da6a 100644

tools/lib/bpf/libbpf_internal.h struct btf_type; · line 238

Quoted 5 lines

tools/lib/bpf/libbpf_internal.h +2 -0

@@ -234,6 +234,8 @@ struct btf_type;
  struct btf_type *btf_type_by_id(const struct btf *btf, __u32 type_id);
  const char *btf_kind_str(const struct btf_type *t);
  const struct btf_type *skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id);
+/* This function is exposed to bpftool */
+const struct btf_type *resolve_func_ptr(const struct btf *btf, __u32 id, __u32 *res_id);

it's trivial helper, there is no need for bpftool to reuse it, let's just implement a local helper for bpftool. We should have done the same with skip_mods_and_typedefs() in gen.c, but oh well, we can fix that later.

Generally speaking, I'd like to minimize amount of internal functions exposed from libbpf to bpftool. It's justified if the logic is non-trivial, but resolve_func_ptr() is not such case.

Quoted 7 lines

tools/lib/bpf/libbpf_internal.h +0 -1

  static inline enum btf_func_linkage btf_func_linkage(const struct btf_type *t)
  {
--

2.34.1

For a struct_ops map, btf_value_type_id is the type ID of it's struct type. This value is required by bpftool to generate skeleton including pointers of shadow types. The code generator gets the type ID from bpf_map__btf_vaule_type_id() in order to get the type information of the struct type of a map.

Signed-off-by Kui-Feng Lee <[email protected]>

---
 tools/lib/bpf/libbpf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +0 -0

index ef8fd20f33ca..465b50235a01 100644

tools/lib/bpf/libbpf.c +0 -1

        }
 
        if (bpf_map_type__is_map_in_map(def->type)) {
--

2.34.1

Quoted 17 lines · Kui-Feng Lee

For a struct_ops map, btf_value_type_id is the type ID of it's struct type. This value is required by bpftool to generate skeleton including pointers of shadow types. The code generator gets the type ID from bpf_map__btf_vaule_type_id() in order to get the type information of the struct type of a map.

Signed-off-by Kui-Feng Lee <[email protected]>

---
  tools/lib/bpf/libbpf.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +0 -0

index ef8fd20f33ca..465b50235a01 100644

tools/lib/bpf/libbpf.c static int init_struct_ops_maps(struct bpf_object *obj, const char *sec_name, · line 1233

Quoted 5 lines

tools/lib/bpf/libbpf.c +1 -0

@@ -1229,6 +1229,7 @@ static int init_struct_ops_maps(struct bpf_object *obj, const char *sec_name,
                 map->name = strdup(var_name);
                 if (!map->name)
                         return -ENOMEM;
+               map->btf_value_type_id = type_id;
 

this part is good

Quoted 9 lines

tools/lib/bpf/libbpf.c +2 -1

                 map->def.type = BPF_MAP_TYPE_STRUCT_OPS;
                 map->def.key_size = sizeof(int);

@@ -4818,7 +4819,9 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
         if (obj->btf && btf__fd(obj->btf) >= 0) {
                 create_attr.btf_fd = btf__fd(obj->btf);
                 create_attr.btf_key_type_id = map->btf_key_type_id;
-               create_attr.btf_value_type_id = map->btf_value_type_id;
+               create_attr.btf_value_type_id =
+                       def->type != BPF_MAP_TYPE_STRUCT_OPS ?

tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4824

Quoted 1 line

tools/lib/bpf/libbpf.c +1 -0

+                       map->btf_value_type_id : 0;

but here I think it's cleaner to reset create_attr.btf_value_type_id to zero a bit lower, see that we have special logic for PERF_EVENT_ARRAY, CGROUP_ARRAY and a bunch more maps. Just add a case for BPF_MAP_TYPE_STRUCT_OPS that will clear create_attr.btf_value_type_id only (keeping btf_fd and map->btf_value_Type_id intact)

Quoted 7 lines

tools/lib/bpf/libbpf.c +0 -1

         }
 
         if (bpf_map_type__is_map_in_map(def->type)) {
--

2.34.1

Convert st_ops->data to the shadow type of the struct_ops map. The shadow type of a struct_ops type is a variant of the original struct type providing a way to access/change the values in the maps of the struct_ops type.

bpf_map__initial_value() will return st_ops->data for struct_ops types. The skeleton is going to use it as the pointer to the shadow type of the original struct type.

One of the main differences between the original struct type and the shadow type is that all function pointers of the shadow type are converted to pointers of struct bpf_program. Users can replace these bpf_program pointers with other BPF programs. The st_ops->progs[] will be updated before updating the value of a map to reflect the changes made by users.

Signed-off-by Kui-Feng Lee <[email protected]>

---
 tools/lib/bpf/libbpf.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +0 -0

index 465b50235a01..2d22344fb127 100644

tools/lib/bpf/libbpf.c +3 -1

                        prog = st_ops->progs[i];
                        if (!prog)
                                continue;

@@ -9308,7 +9311,9 @@ static struct bpf_map *find_struct_ops_map_by_offset(struct bpf_object *obj,
        return NULL;
 }
 
-/* Collect the reloc from ELF and populate the st_ops->progs[] */
+/* Collect the reloc from ELF, populate the st_ops->progs[], and update
+ * st_ops->data for shadow type.
+ */
 static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
                                            Elf64_Shdr *shdr, Elf_Data *data)
 {

tools/lib/bpf/libbpf.c +12 -1

+                * bpf_map__initial_value() as a pointer to the shadow
+                * type. All function pointers in the original struct type
+                * should be converted to a pointer to struct bpf_program
+                * in the shadow type.
+                */
+               *((struct bpf_program **)(st_ops->data + moff)) = prog;
        }
 
        return 0;

@@ -9880,6 +9893,12 @@ int bpf_map__set_initial_value(struct bpf_map *map,
 
 void *bpf_map__initial_value(struct bpf_map *map, size_t *psize)
 {
+       if (bpf_map__is_struct_ops(map)) {
+               if (psize)
+                       *psize = map->def.value_size;
+               return map->st_ops->data;
+       }
+
        if (!map->mmaped)
                return NULL;
        *psize = map->def.value_size;
--

2.34.1

Also, even if the bpf_program pointer is correct, it could be a program of the wrong type, so I think we should add a bit more validation here, given these pointers are set by users directly after bpf_object is opened.

Quoted 26 lines · Kui-Feng Lee

Convert st_ops->data to the shadow type of the struct_ops map. The shadow type of a struct_ops type is a variant of the original struct type providing a way to access/change the values in the maps of the struct_ops type.

bpf_map__initial_value() will return st_ops->data for struct_ops types. The skeleton is going to use it as the pointer to the shadow type of the original struct type.

One of the main differences between the original struct type and the shadow type is that all function pointers of the shadow type are converted to pointers of struct bpf_program. Users can replace these bpf_program pointers with other BPF programs. The st_ops->progs[] will be updated before updating the value of a map to reflect the changes made by users.

Signed-off-by Kui-Feng Lee <[email protected]>

---
  tools/lib/bpf/libbpf.c | 21 ++++++++++++++++++++-
  1 file changed, 20 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +0 -0

index 465b50235a01..2d22344fb127 100644

tools/lib/bpf/libbpf.c static int bpf_map__init_kern_struct_ops(struct bpf_map *map) · line 1107

Quoted 6 lines

tools/lib/bpf/libbpf.c +3 -0

@@ -1102,6 +1102,9 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
                 if (btf_is_ptr(mtype)) {
                         struct bpf_program *prog;
 
+                       /* Update the value from the shadow type */
+                       st_ops->progs[i] = *(struct bpf_program **)mdata;
+

it's unsettling to just cast a pointer like this without any validation. It's too easy for users to set either some garbage there or struct bpf_program * pointer from some other skeleton.

Luckily, validation is pretty simple, we can just iterate over all bpf_object's programs and check if any of them matches the value of the mdata pointer. If not, error out with meaningful error.

Also, even if the bpf_program pointer is correct, it could be a program of the wrong type, so I think we should add a bit more validation here, given these pointers are set by users directly after bpf_object is opened.

Quoted 16 lines

tools/lib/bpf/libbpf.c +3 -1

                         prog = st_ops->progs[i];
                         if (!prog)
                                 continue;

@@ -9308,7 +9311,9 @@ static struct bpf_map *find_struct_ops_map_by_offset(struct bpf_object *obj,
         return NULL;
  }
 
-/* Collect the reloc from ELF and populate the st_ops->progs[] */
+/* Collect the reloc from ELF, populate the st_ops->progs[], and update
+ * st_ops->data for shadow type.
+ */
  static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
                                             Elf64_Shdr *shdr, Elf_Data *data)
  {

tools/lib/bpf/libbpf.c static int bpf_object__collect_st_ops_relos(struct bpf_object *obj, · line 9431

Quoted 5 lines

tools/lib/bpf/libbpf.c +2 -0

@@ -9422,6 +9427,14 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
                 }
 
                 st_ops->progs[member_idx] = prog;
+
+               /* st_ops->data will be expose to users, being returned by

typo: exposed

Quoted 25 lines

tools/lib/bpf/libbpf.c +12 -1

+                * bpf_map__initial_value() as a pointer to the shadow
+                * type. All function pointers in the original struct type
+                * should be converted to a pointer to struct bpf_program
+                * in the shadow type.
+                */
+               *((struct bpf_program **)(st_ops->data + moff)) = prog;
         }
 
         return 0;

@@ -9880,6 +9893,12 @@ int bpf_map__set_initial_value(struct bpf_map *map,
 
  void *bpf_map__initial_value(struct bpf_map *map, size_t *psize)
  {
+       if (bpf_map__is_struct_ops(map)) {
+               if (psize)
+                       *psize = map->def.value_size;
+               return map->st_ops->data;
+       }
+
         if (!map->mmaped)
                 return NULL;
         *psize = map->def.value_size;
--

2.34.1

Declares and defines a pointer of the shadow type for each struct_ops map.

The code generator will create an anonymous struct type as the shadow type for each struct_ops map. The shadow type is translated from the original struct type of the map. The user of the skeleton use pointers of them to access the values of struct_ops maps.

However, shadow types only supports certain types of fields, including scalar types and function pointers. Any fields of unsupported types are translated into an array of characters to occupy the space of the original field. Function pointers are translated into pointers of the struct bpf_program. Additionally, padding fields are generated to occupy the space between two consecutive fields.

The pointers of shadow types of struct_osp maps are initialized when *__open_opts() in skeletons are called. For a map called FOO, the user can access it through the pointer at skel->struct_ops.FOO.

Reviewed-by Quentin Monnet <[email protected]>

Signed-off-by Kui-Feng Lee <[email protected]>

---
 tools/bpf/bpftool/gen.c | 235 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 234 insertions(+), 1 deletion(-)

tools/bpf/bpftool/gen.c +0 -0

index a9334c57e859..a21c92d95401 100644

tools/bpf/bpftool/gen.c +13 -0

+       const struct btf_type *map_type, *member_type;
+       __u32 map_type_id, member_type_id;
+       __u32 offset, next_offset = 0;
+       const struct btf_member *m;
+       const char *member_name;
+       struct btf_dump *d = NULL;
+       int i, err = 0;
+       int size, map_size;
+
+       map_type_id = bpf_map__btf_value_type_id(map);
+       if (map_type_id == 0)
+               return -EINVAL;
+       map_type = btf__type_by_id(btf, map_type_id);

tools/bpf/bpftool/gen.c +7 -0

+                       if (err)
+                               goto out;
+                       printf(";\n");
+
+                       size = btf__resolve_size(btf, member_type_id);
+                       if (size < 0) {
+                               err = size;

tools/bpf/bpftool/gen.c +20 -0

+                                      member_name);
+
+                               next_offset = offset + sizeof(void *);
+                               break;
+                       }
+                       /* All pointer types are unsupported except for
+                        * function pointers.
+                        */
+                       fallthrough;
+
+               default:
+                       /* Unsupported types
+                        *
+                        * Types other than scalar types and function
+                        * pointers are currently not supported in order to
+                        * prevent conflicts in the generated code caused
+                        * by multiple definitions. For instance, if the
+                        * struct type FOO is used in a struct_ops map,
+                        * bpftool has to generate definitions for FOO,
+                        * which may result in conflicts if FOO is defined

tools/bpf/bpftool/gen.c +11 -0

+
+out:
+       btf_dump__free(d);
+
+       return err;
+}
+
+/* Generate the pointer of the shadow type for a struct_ops map.
+ *
+ * This function adds a pointer of the shadow type for a struct_ops map.
+ * The members of a struct_ops map can be exported through a pointer to a

tools/bpf/bpftool/gen.c +42 -0

+
+       err = walk_st_ops_shadow_vars(btf, ident, map);
+       if (err)
+               return err;
+
+       printf("\t\t} *%s;\n", ident);
+
+       return 0;
+}
+
+static int gen_st_ops_shadow(struct btf *btf, struct bpf_object *obj)
+{
+       struct bpf_map *map;
+       char ident[256];
+       int err;
+
+       /* Generate the pointers to shadow types of
+        * struct_ops maps.
+        */
+       printf("\tstruct {\n");
+       bpf_object__for_each_map(map, obj) {
+               if (bpf_map__type(map) != BPF_MAP_TYPE_STRUCT_OPS)
+                       continue;
+               if (!get_map_ident(map, ident, sizeof(ident)))
+                       continue;
+               err = gen_st_ops_shadow_type(btf, ident, map);
+               if (err)
+                       return err;
+       }
+       printf("\t} struct_ops;\n");
+
+       return 0;
+}
+
+/* Generate the code to initialize the pointers of shadow types. */
+static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj)
+{
+       struct bpf_map *map;
+       char ident[256];
+
+       /* Initialize the pointers to_ops shadow types of
+        * struct_ops maps.

tools/bpf/bpftool/gen.c +8 -0

+                       \n\
+                       ", ident);
+       }
+}
+
 static int do_skeleton(int argc, char **argv)
 {
        char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];

@@ -923,6 +1124,7 @@ static int do_skeleton(int argc, char **argv)
        struct bpf_map *map;
        struct btf *btf;
        struct stat st;
+       int st_ops_cnt = 0;
 
        if (!REQ_ARGS(1)) {
                usage();

@@ -1039,6 +1241,8 @@ static int do_skeleton(int argc, char **argv)
                );
        }
 
+       btf = bpf_object__btf(obj);
+
        if (map_cnt) {
                printf("\tstruct {\n");
                bpf_object__for_each_map(map, obj) {

@@ -1048,8 +1252,15 @@ static int do_skeleton(int argc, char **argv)
                                printf("\t\tstruct bpf_map_desc %s;\n", ident);
                        else

tools/bpf/bpftool/gen.c +0 -1

        }
 
        if (prog_cnt) {

@@ -1075,7 +1286,6 @@ static int do_skeleton(int argc, char **argv)
                printf("\t} links;\n");
        }
 
-       btf = bpf_object__btf(obj);
        if (btf) {
                err = codegen_datasecs(obj, obj_name);
                if (err)

tools/bpf/bpftool/gen.c +4 -0

+
+       codegen("\
+               \n\
                        return obj;                                         \n\
                err_out:                                                    \n\
                        %1$s__destroy(obj);                                 \n\

@@ -1296,6 +1513,7 @@ static int do_subskeleton(int argc, char **argv)
        struct btf *btf;
        const struct btf_type *map_type, *var_type;
        const struct btf_var_secinfo *var;
+       int st_ops_cnt = 0;
        struct stat st;
 
        if (!REQ_ARGS(1)) {

tools/bpf/bpftool/gen.c +13 -1

                }
                printf("\t} maps;\n");
        }
 
+       if (st_ops_cnt && btf) {
+               err = gen_st_ops_shadow(btf, obj);
+               if (err)
+                       goto out;
+       }
+
        if (prog_cnt) {
                printf("\tstruct {\n");
                bpf_object__for_each_program(prog, obj) {

@@ -1553,6 +1779,13 @@ static int do_subskeleton(int argc, char **argv)
                        if (err)                                            \n\
                                goto err;                                   \n\
                                                                            \n\
+               ");
+
+       if (st_ops_cnt && btf)
+               gen_st_ops_shadow_init(btf, obj);
+
+       codegen("\
+               \n\
                        return obj;                                         \n\
                err:                                                        \n\
                        %1$s__destroy(obj);                                 \n\
--

2.34.1

tools/bpf/bpftool/gen.c codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li · line 1055

Quoted 10 lines · Kui-Feng Lee

+ * type. Accessing them through the generated names may unintentionally
+ * corrupt data.
+ */

tools/bpf/bpftool/gen.c +6 -0

+static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
+                                 const struct bpf_map *map)
+{
+       int err;
+
+       printf("\t\tstruct {\n");

would it be useful to still name this type? E.g., if it is struct bpf_struct_ops_tcp_congestion_ops in vmlinux BTF we can name this one as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a similar pattern for bss/data/rodata sections, having names is useful.

Quoted 11 lines

tools/bpf/bpftool/gen.c +10 -0

+
+       err = walk_st_ops_shadow_vars(btf, ident, map);
+       if (err)
+               return err;
+
+       printf("\t\t} *%s;\n", ident);
+
+       return 0;
+}
+

Will fix most of issues.

Quoted 16 lines · Andrii Nakryiko

tools/bpf/bpftool/gen.c codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li · line 1055

Quoted 10 lines · Kui-Feng Lee

+ * type. Accessing them through the generated names may unintentionally
+ * corrupt data.
+ */

tools/bpf/bpftool/gen.c +6 -0

+static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
+                                 const struct bpf_map *map)
+{
+       int err;
+
+       printf("\t\tstruct {\n");

would it be useful to still name this type? E.g., if it is struct bpf_struct_ops_tcp_congestion_ops in vmlinux BTF we can name this one as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a similar pattern for bss/data/rodata sections, having names is useful.

If a user defines several struct_ops maps with the same name and type in different files, it can cause name conflicts. Unless we also prefix the name with the name of the skeleton. I am not sure if it is a good idea to generate such long names. If a user want to refer to the type, he still can use typeof(). WDYT?

Quoted 12 lines

Quoted 12 lines

tools/bpf/bpftool/gen.c +10 -0

+
+       err = walk_st_ops_shadow_vars(btf, ident, map);
+       if (err)
+               return err;
+
+       printf("\t\t} *%s;\n", ident);
+
+       return 0;
+}
+

Quoted 21 lines · Kui-Feng Lee

Will fix most of issues.

Quoted 17 lines · Andrii Nakryiko

tools/bpf/bpftool/gen.c codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li · line 1055

Quoted 10 lines · Kui-Feng Lee

+ * type. Accessing them through the generated names may unintentionally
+ * corrupt data.
+ */

tools/bpf/bpftool/gen.c +6 -0

+static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
+                                 const struct bpf_map *map)
+{
+       int err;
+
+       printf("\t\tstruct {\n");

would it be useful to still name this type? E.g., if it is struct bpf_struct_ops_tcp_congestion_ops in vmlinux BTF we can name this one as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a similar pattern for bss/data/rodata sections, having names is useful.

Quoted 5 lines

If a user defines several struct_ops maps with the same name and type in different files, it can cause name conflicts. Unless we also prefix the name with the name of the skeleton. I am not sure if it is a good idea to generate such long names. If a user want to refer to the type, he still can use typeof(). WDYT?

I misread your words. So, you were saying to prefix the skeleton name, not map names. It is doable.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

tools/bpf/bpftool/gen.c +10 -0

+
+       err = walk_st_ops_shadow_vars(btf, ident, map);
+       if (err)
+               return err;
+
+       printf("\t\t} *%s;\n", ident);
+
+       return 0;
+}
+

Quoted 31 lines · Kui-Feng Lee

Quoted 26 lines · Kui-Feng Lee

Will fix most of issues.

Quoted 17 lines · Andrii Nakryiko

tools/bpf/bpftool/gen.c codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li · line 1055

Quoted 10 lines · Kui-Feng Lee

+ * type. Accessing them through the generated names may unintentionally
+ * corrupt data.
+ */

tools/bpf/bpftool/gen.c +6 -0

+static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
+                                 const struct bpf_map *map)
+{
+       int err;
+
+       printf("\t\tstruct {\n");

would it be useful to still name this type? E.g., if it is struct bpf_struct_ops_tcp_congestion_ops in vmlinux BTF we can name this one as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a similar pattern for bss/data/rodata sections, having names is useful.

If a user defines several struct_ops maps with the same name and type in different files, it can cause name conflicts. Unless we also prefix the name with the name of the skeleton. I am not sure if it is a good idea to generate such long names. If a user want to refer to the type, he still can use typeof(). WDYT?

Quoted 2 lines

I misread your words. So, you were saying to prefix the skeleton name, not map names. It is doable.

I did say to prefix with skeleton name, but *that* actually can lead to a conflict if you have two struct_ops maps that use the same BTF type. On the other hand, map names are unique, they are forced to be global symbols, so there is no way for them to conflict (it would be link-time error).

How about we append both skeleton name, map name, and map's underlying BTF type? So:

<skel>__<map>__bpf_struct_ops_tcp_congestion_ops

?

Is there any problem with having a long name?

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

tools/bpf/bpftool/gen.c +10 -0

+
+       err = walk_st_ops_shadow_vars(btf, ident, map);
+       if (err)
+               return err;
+
+       printf("\t\t} *%s;\n", ident);
+
+       return 0;
+}
+

Quoted 34 lines · Andrii Nakryiko

Quoted 33 lines · Kui-Feng Lee

Quoted 28 lines · Kui-Feng Lee

Will fix most of issues.

Quoted 19 lines · Andrii Nakryiko

tools/bpf/bpftool/gen.c codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li · line 1055

Quoted 12 lines · Kui-Feng Lee

+ * type. Accessing them through the generated names may unintentionally
+ * corrupt data.
+ */
+static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
+                                 const struct bpf_map *map)

tools/bpf/bpftool/gen.c +4 -0

+{
+       int err;
+
+       printf("\t\tstruct {\n");

would it be useful to still name this type? E.g., if it is struct bpf_struct_ops_tcp_congestion_ops in vmlinux BTF we can name this one as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a similar pattern for bss/data/rodata sections, having names is useful.

If a user defines several struct_ops maps with the same name and type in different files, it can cause name conflicts. Unless we also prefix the name with the name of the skeleton. I am not sure if it is a good idea to generate such long names. If a user want to refer to the type, he still can use typeof(). WDYT?

Quoted 8 lines

Quoted 2 lines

I misread your words. So, you were saying to prefix the skeleton name, not map names. It is doable.

I did say to prefix with skeleton name, but *that* actually can lead to a conflict if you have two struct_ops maps that use the same BTF type. On the other hand, map names are unique, they are forced to be global symbols, so there is no way for them to conflict (it would be link-time error).

I avoided conflicts by checking if the definition of a type is already generated.

For example, if there are two maps with the same type, they would looks like. struct XXXSekelton {
    ...
    struct {
        struct struct_ops_type {
             ....
        } *map1;
        struct struct_ops_type *map2;
    } struct_ops;
  ...
};

WDYT?

Quoted 10 lines

How about we append both skeleton name, map name, and map's underlying BTF type? So:

<skel>__<map>__bpf_struct_ops_tcp_congestion_ops

?

Is there any problem with having a long name?

No a big problem! Just not convenient to use.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

tools/bpf/bpftool/gen.c +10 -0

+
+       err = walk_st_ops_shadow_vars(btf, ident, map);
+       if (err)
+               return err;
+
+       printf("\t\t} *%s;\n", ident);
+
+       return 0;
+}
+

Quoted 32 lines · Andrii Nakryiko

Quoted 31 lines · Kui-Feng Lee

Quoted 26 lines · Kui-Feng Lee

Will fix most of issues.

Quoted 17 lines · Andrii Nakryiko

tools/bpf/bpftool/gen.c codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li · line 1055

Quoted 10 lines · Kui-Feng Lee

+ * type. Accessing them through the generated names may unintentionally
+ * corrupt data.
+ */

tools/bpf/bpftool/gen.c +6 -0

+static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
+                                 const struct bpf_map *map)
+{
+       int err;
+
+       printf("\t\tstruct {\n");

would it be useful to still name this type? E.g., if it is struct bpf_struct_ops_tcp_congestion_ops in vmlinux BTF we can name this one as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a similar pattern for bss/data/rodata sections, having names is useful.

If a user defines several struct_ops maps with the same name and type in different files, it can cause name conflicts. Unless we also prefix the name with the name of the skeleton. I am not sure if it is a good idea to generate such long names. If a user want to refer to the type, he still can use typeof(). WDYT?

Quoted 8 lines

Quoted 2 lines

I misread your words. So, you were saying to prefix the skeleton name, not map names. It is doable.

I did say to prefix with skeleton name, but *that* actually can lead to a conflict if you have two struct_ops maps that use the same BTF type. On the other hand, map names are unique, they are forced to be global symbols, so there is no way for them to conflict (it would be link-time error).

I avoided conflicts by checking if the definition of a type is already generated.

For example, if there are two maps with the same type, they would looks like.

struct XXXSekelton {
     ...
     struct {
         struct struct_ops_type {
              ....
         } *map1;
         struct struct_ops_type *map2;

     } struct_ops;

...
};

WDYT?

Quoted 9 lines

How about we append both skeleton name, map name, and map's underlying BTF type? So:

<skel>__<map>__bpf_struct_ops_tcp_congestion_ops

?

Is there any problem with having a long name?

No a big problem! Just not convenient to use.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

tools/bpf/bpftool/gen.c +10 -0

+
+       err = walk_st_ops_shadow_vars(btf, ident, map);
+       if (err)
+               return err;
+
+       printf("\t\t} *%s;\n", ident);
+
+       return 0;
+}
+

Quoted 50 lines · Kui-Feng Lee

Quoted 32 lines · Andrii Nakryiko

Quoted 31 lines · Kui-Feng Lee

Quoted 26 lines · Kui-Feng Lee

Will fix most of issues.

Quoted 17 lines · Andrii Nakryiko

Review

Quoted 10 lines · Kui-Feng Lee

+ * type. Accessing them through the generated names may unintentionally
+ * corrupt data.
+ */

(quoted diff) +6 -0

+static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
+                                 const struct bpf_map *map)
+{
+       int err;
+
+       printf("\t\tstruct {\n");

would it be useful to still name this type? E.g., if it is struct bpf_struct_ops_tcp_congestion_ops in vmlinux BTF we can name this one as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a similar pattern for bss/data/rodata sections, having names is useful.

If a user defines several struct_ops maps with the same name and type in different files, it can cause name conflicts. Unless we also prefix the name with the name of the skeleton. I am not sure if it is a good idea to generate such long names. If a user want to refer to the type, he still can use typeof(). WDYT?

Quoted 8 lines

Quoted 2 lines

I misread your words. So, you were saying to prefix the skeleton name, not map names. It is doable.

I did say to prefix with skeleton name, but *that* actually can lead to a conflict if you have two struct_ops maps that use the same BTF type. On the other hand, map names are unique, they are forced to be global symbols, so there is no way for them to conflict (it would be link-time error).

I avoided conflicts by checking if the definition of a type is already generated.

For example, if there are two maps with the same type, they would looks like.

Quoted 7 lines

struct XXXSekelton {
      ...
      struct {
          struct struct_ops_type {
               ....
          } *map1;
          struct struct_ops_type *map2;

It's kind of non-uniform. I think we are overengineering this, let's just do <skel>__<map>__<type> and see how it goes? No checks, no nothing, pure string generation.

Quoted 33 lines

      } struct_ops;
    ...
};

WDYT?

Quoted 9 lines

How about we append both skeleton name, map name, and map's underlying BTF type? So:

<skel>__<map>__bpf_struct_ops_tcp_congestion_ops

?

Is there any problem with having a long name?

No a big problem! Just not convenient to use.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

(quoted diff) +10 -0

+
+       err = walk_st_ops_shadow_vars(btf, ident, map);
+       if (err)
+               return err;
+
+       printf("\t\t} *%s;\n", ident);
+
+       return 0;
+}
+

The example in bpftool-gen.8 explains how to use the pointer of the shadow type to change the value of a field of a struct_ops map.

Signed-off-by Kui-Feng Lee <[email protected]>