* [PATCH bpf-next v5 0/6] Create shadow types for struct_ops maps in skeletons
@ 2024-02-27 1:04 Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h Kui-Feng Lee
` (5 more replies)
0 siblings, 6 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-27 1:04 UTC (permalink / raw
To: bpf, ast, martin.lau, song, kernel-team, andrii, quentin
Cc: sinquersw, kuifeng, Kui-Feng Lee
This patchset allows skeleton users to change the values of the fields
in struct_ops maps at runtime. It will create a shadow type pointer in
a skeleton for each struct_ops map, allowing users to access the
values of fields through these pointers. For instance, if there is an
integer field named "FOO" in a struct_ops map called "testmap", you
can access the value of "FOO" in this way.
skel->struct_ops.testmap->FOO = 13;
With this feature, the users can pass flags or other data along with
the map from the user space to the kernel without creating separate
struct_ops map for different values in BPF.
== Shadow Type ==
The shadow type of a struct_ops map is a variant of the original
struct type of the map. The code generator translates each field in
the original struct type to a field in the shadow type. The type of a
field in the shadow type may not be the same as the corresponding
field in the original struct type. For example, modifiers like
volatile, const, etc., are removed from the fields in a shadow
type. Function pointers are translated to pointers of struct
bpf_program.
Currently, only scalar types and function pointers are
supported. Fields belonging to structs, unions, non-function pointers,
arrays, or other types are not supported. For those unsupported
fields, they are converted to arrays of characters to preserve their
space within the original struct type.
The padding between consecutive fields is handled by padding fields
(__padding_*). This helps to maintain the memory layout consistent
with the original struct_type.
Here is an example of shadow types.
The origin struct type of a struct_ops map is
struct bpf_testmod_ops {
int (*test_1)(void);
void (*test_2)(int a, int b);
/* Used to test nullable arguments. */
int (*test_maybe_null)(int dummy, struct task_struct *task);
/* The following fields are used to test shadow copies. */
char onebyte;
struct {
int a;
int b;
} unsupported;
int data;
};
The struct_ops map, named testmod_1, of this type will be translated
to a pointer in the shadow type.
struct {
struct {
const struct bpf_program *test_1;
const struct bpf_program *test_2;
const struct bpf_program *test_maybe_null;
char onebyte;
char __padding_3[3];
char __padding_4[8];
int data;
} *testmod_1;
} struct_ops;
== Convert st_ops->data to Shadow Type ==
libbpf converts st_ops->data to the format of the shadow type for each
struct_ops map. This means that the bytes where function pointers are
located are converted to the values of the pointers of struct
bpf_program. The fields of other types are kept as they were.
Libbpf will synchronize the pointers of struct bpf_program with
st_ops->progs[] so that users can change function pointers
(bpf_program) before loading the map.
---
Changes from v4:
- Convert function pointers to the pointers to struct bpf_program in
bpf_object__collect_st_ops_relos().
Changes from v3:
- Add comment to avoid people from removing resolve_func_ptr() from
libbpf_internal.h
- Fix commit logs and comments.
- Add an example about using the pointers of shadow types
for struct_ops maps to bpftool-gen.8.
v4: https://lore.kernel.org/all/20240222222624.1163754-1-thinker.li@gmail.com/
v3: https://lore.kernel.org/all/20240221012329.1387275-1-thinker.li@gmail.com/
v2: https://lore.kernel.org/all/20240214020836.1845354-1-thinker.li@gmail.com/
v1: https://lore.kernel.org/all/20240124224130.859921-1-thinker.li@gmail.com/
Kui-Feng Lee (6):
libbpf: expose resolve_func_ptr() through libbpf_internal.h.
libbpf: set btf_value_type_id of struct bpf_map for struct_ops.
libbpf: Convert st_ops->data to shadow type.
bpftool: generated shadow variables for struct_ops maps.
bpftool: Add an example for struct_ops map and shadow type.
selftests/bpf: Test if shadow types work correctly.
.../bpf/bpftool/Documentation/bpftool-gen.rst | 58 ++++-
tools/bpf/bpftool/gen.c | 235 +++++++++++++++++-
tools/lib/bpf/libbpf.c | 28 ++-
tools/lib/bpf/libbpf_internal.h | 2 +
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 11 +-
.../selftests/bpf/bpf_testmod/bpf_testmod.h | 8 +
.../bpf/prog_tests/test_struct_ops_module.c | 19 +-
.../selftests/bpf/progs/struct_ops_module.c | 8 +
8 files changed, 354 insertions(+), 15 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h.
2024-02-27 1:04 [PATCH bpf-next v5 0/6] Create shadow types for struct_ops maps in skeletons Kui-Feng Lee
@ 2024-02-27 1:04 ` Kui-Feng Lee
2024-02-28 14:38 ` Quentin Monnet
2024-02-28 17:45 ` Andrii Nakryiko
2024-02-27 1:04 ` [PATCH bpf-next v5 2/6] libbpf: set btf_value_type_id of struct bpf_map for struct_ops Kui-Feng Lee
` (4 subsequent siblings)
5 siblings, 2 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-27 1:04 UTC (permalink / raw
To: bpf, ast, martin.lau, song, kernel-team, andrii, quentin
Cc: sinquersw, kuifeng, 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 <thinker.li@gmail.com>
---
tools/lib/bpf/libbpf.c | 2 +-
tools/lib/bpf/libbpf_internal.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 01f407591a92..ef8fd20f33ca 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -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;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index ad936ac5e639..17e6d381da6a 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -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);
static inline enum btf_func_linkage btf_func_linkage(const struct btf_type *t)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next v5 2/6] libbpf: set btf_value_type_id of struct bpf_map for struct_ops.
2024-02-27 1:04 [PATCH bpf-next v5 0/6] Create shadow types for struct_ops maps in skeletons Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h Kui-Feng Lee
@ 2024-02-27 1:04 ` Kui-Feng Lee
2024-02-28 17:48 ` Andrii Nakryiko
2024-02-27 1:04 ` [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type Kui-Feng Lee
` (3 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-27 1:04 UTC (permalink / raw
To: bpf, ast, martin.lau, song, kernel-team, andrii, quentin
Cc: sinquersw, kuifeng, 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 <thinker.li@gmail.com>
---
tools/lib/bpf/libbpf.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ef8fd20f33ca..465b50235a01 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -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;
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 ?
+ map->btf_value_type_id : 0;
}
if (bpf_map_type__is_map_in_map(def->type)) {
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type.
2024-02-27 1:04 [PATCH bpf-next v5 0/6] Create shadow types for struct_ops maps in skeletons Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 2/6] libbpf: set btf_value_type_id of struct bpf_map for struct_ops Kui-Feng Lee
@ 2024-02-27 1:04 ` Kui-Feng Lee
2024-02-28 17:58 ` Andrii Nakryiko
2024-02-27 1:04 ` [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps Kui-Feng Lee
` (2 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-27 1:04 UTC (permalink / raw
To: bpf, ast, martin.lau, song, kernel-team, andrii, quentin
Cc: sinquersw, kuifeng, 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 <thinker.li@gmail.com>
---
tools/lib/bpf/libbpf.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 465b50235a01..2d22344fb127 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -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;
+
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)
{
@@ -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
+ * 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-27 1:04 [PATCH bpf-next v5 0/6] Create shadow types for struct_ops maps in skeletons Kui-Feng Lee
` (2 preceding siblings ...)
2024-02-27 1:04 ` [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type Kui-Feng Lee
@ 2024-02-27 1:04 ` Kui-Feng Lee
2024-02-28 18:25 ` Andrii Nakryiko
2024-02-27 1:04 ` [PATCH bpf-next v5 5/6] bpftool: Add an example for struct_ops map and shadow type Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 6/6] selftests/bpf: Test if shadow types work correctly Kui-Feng Lee
5 siblings, 1 reply; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-27 1:04 UTC (permalink / raw
To: bpf, ast, martin.lau, song, kernel-team, andrii, quentin
Cc: sinquersw, kuifeng, Kui-Feng Lee
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 <quentin@isovalent.com>
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
tools/bpf/bpftool/gen.c | 235 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 234 insertions(+), 1 deletion(-)
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index a9334c57e859..a21c92d95401 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -909,6 +909,207 @@ codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li
}
}
+static int walk_st_ops_shadow_vars(struct btf *btf,
+ const char *ident,
+ const struct bpf_map *map)
+{
+ DECLARE_LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts,
+ .indent_level = 3,
+ );
+ 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);
+ if (!map_type)
+ return -EINVAL;
+
+ d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
+ if (!d)
+ return -errno;
+
+ for (i = 0, m = btf_members(map_type);
+ i < btf_vlen(map_type);
+ i++, m++) {
+ member_type = skip_mods_and_typedefs(btf, m->type,
+ &member_type_id);
+ if (!member_type) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ member_name = btf__name_by_offset(btf, m->name_off);
+ if (!member_name) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ offset = m->offset / 8;
+ if (next_offset != offset) {
+ printf("\t\t\tchar __padding_%d[%d];\n",
+ i - 1, offset - next_offset);
+ }
+
+ switch (btf_kind(member_type)) {
+ case BTF_KIND_INT:
+ case BTF_KIND_FLOAT:
+ case BTF_KIND_ENUM:
+ case BTF_KIND_ENUM64:
+ /* scalar type */
+ printf("\t\t\t");
+ opts.field_name = member_name;
+ err = btf_dump__emit_type_decl(d, member_type_id,
+ &opts);
+ if (err)
+ goto out;
+ printf(";\n");
+
+ size = btf__resolve_size(btf, member_type_id);
+ if (size < 0) {
+ err = size;
+ goto out;
+ }
+
+ next_offset = offset + size;
+ break;
+
+ case BTF_KIND_PTR:
+ if (resolve_func_ptr(btf, m->type, NULL)) {
+ /* Function pointer */
+ printf("\t\t\tconst struct bpf_program *%s;\n",
+ 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
+ * in different skeleton files.
+ */
+ if (i == btf_vlen(map_type) - 1) {
+ map_size = btf__resolve_size(btf, map_type_id);
+ if (map_size < 0)
+ return -EINVAL;
+ size = map_size - offset;
+ } else {
+ size = (m[1].offset - m->offset) / 8;
+ }
+
+ printf("\t\t\tchar __padding_%d[%d];\n", i, size);
+
+ next_offset = offset + size;
+ break;
+ }
+ }
+
+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
+ * shadow type. The user can access these members through the pointer.
+ *
+ * A shadow type includes not all members, only members of some types.
+ * They are scalar types and function pointers. The function pointers are
+ * translated to the pointer of the struct bpf_program. The scalar types
+ * are translated to the original type without any modifiers.
+ *
+ * Unsupported types will be translated to a char array to occupy the same
+ * space as the original field. However, the user should not access them
+ * directly. These unsupported fields are also renamed as __padding_*
+ * . They may be reordered or shifted due to changes in the original struct
+ * 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)
+{
+ int err;
+
+ printf("\t\tstruct {\n");
+
+ 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.
+ */
+ 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;
+ codegen("\
+ \n\
+ obj->struct_ops.%1$s = \n\
+ bpf_map__initial_value(obj->maps.%1$s, NULL);\n\
+ \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
printf("\t\tstruct bpf_map *%s;\n", ident);
+ if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
+ st_ops_cnt++;
}
printf("\t} maps;\n");
+ if (st_ops_cnt && btf) {
+ err = gen_st_ops_shadow(btf, obj);
+ if (err)
+ goto out;
+ }
}
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)
@@ -1133,6 +1343,13 @@ static int do_skeleton(int argc, char **argv)
if (err) \n\
goto err_out; \n\
\n\
+ ", obj_name);
+
+ if (st_ops_cnt && btf)
+ gen_st_ops_shadow_init(btf, obj);
+
+ 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)) {
@@ -1438,10 +1656,18 @@ static int do_subskeleton(int argc, char **argv)
if (!get_map_ident(map, ident, sizeof(ident)))
continue;
printf("\t\tstruct bpf_map *%s;\n", ident);
+ if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
+ st_ops_cnt++;
}
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next v5 5/6] bpftool: Add an example for struct_ops map and shadow type.
2024-02-27 1:04 [PATCH bpf-next v5 0/6] Create shadow types for struct_ops maps in skeletons Kui-Feng Lee
` (3 preceding siblings ...)
2024-02-27 1:04 ` [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps Kui-Feng Lee
@ 2024-02-27 1:04 ` Kui-Feng Lee
2024-02-28 14:38 ` Quentin Monnet
2024-02-27 1:04 ` [PATCH bpf-next v5 6/6] selftests/bpf: Test if shadow types work correctly Kui-Feng Lee
5 siblings, 1 reply; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-27 1:04 UTC (permalink / raw
To: bpf, ast, martin.lau, song, kernel-team, andrii, quentin
Cc: sinquersw, kuifeng, Kui-Feng Lee
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 <thinker.li@gmail.com>
---
.../bpf/bpftool/Documentation/bpftool-gen.rst | 58 +++++++++++++++++--
1 file changed, 52 insertions(+), 6 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
index 5006e724d1bc..62572f5beed9 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-gen.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
@@ -257,18 +257,48 @@ EXAMPLES
return 0;
}
-This is example BPF application with two BPF programs and a mix of BPF maps
-and global variables. Source code is split across two source code files.
+**$ cat example3.bpf.c**
+
+::
+
+ #include <linux/ptrace.h>
+ #include <linux/bpf.h>
+ #include <bpf/bpf_helpers.h>
+ /* This header file is provided by the bpf_testmod module. */
+ #include "bpf_testmod.h"
+
+ int test_2_result = 0;
+
+ /* bpf_Testmod.ko calls this function, passing a "4"
+ * and testmod_map->data.
+ */
+ SEC("struct_ops/test_2")
+ void BPF_PROG(test_2, int a, int b)
+ {
+ test_2_result = a + b;
+ }
+
+ SEC(".struct_ops")
+ struct bpf_testmod_ops testmod_map = {
+ .test_2 = (void *)test_2,
+ .data = 0x1,
+ };
+
+This is example BPF application with three BPF programs and a mix of BPF
+maps and global variables. Source code is split across three source code
+files.
**$ clang --target=bpf -g example1.bpf.c -o example1.bpf.o**
**$ clang --target=bpf -g example2.bpf.c -o example2.bpf.o**
-**$ bpftool gen object example.bpf.o example1.bpf.o example2.bpf.o**
+**$ clang --target=bpf -g example3.bpf.c -o example3.bpf.o**
+
+**$ bpftool gen object example.bpf.o example1.bpf.o example2.bpf.o example3.bpf.o**
-This set of commands compiles *example1.bpf.c* and *example2.bpf.c*
-individually and then statically links respective object files into the final
-BPF ELF object file *example.bpf.o*.
+This set of commands compiles *example1.bpf.c*, *example2.bpf.c* and
+*example3.bpf.c* individually and then statically links respective object
+files into the final BPF ELF object file *example.bpf.o*.
**$ bpftool gen skeleton example.bpf.o name example | tee example.skel.h**
@@ -291,7 +321,15 @@ BPF ELF object file *example.bpf.o*.
struct bpf_map *data;
struct bpf_map *bss;
struct bpf_map *my_map;
+ struct bpf_map *testmod_map;
} maps;
+ struct {
+ struct {
+ const struct bpf_program *test_1;
+ const struct bpf_program *test_2;
+ int data;
+ } *testmod_map;
+ } struct_ops;
struct {
struct bpf_program *handle_sys_enter;
struct bpf_program *handle_sys_exit;
@@ -304,6 +342,7 @@ BPF ELF object file *example.bpf.o*.
struct {
int x;
} data;
+ int test_2_result;
} *bss;
struct example__data {
_Bool global_flag;
@@ -342,10 +381,16 @@ BPF ELF object file *example.bpf.o*.
skel->rodata->param1 = 128;
+ /* Change the value through the pointer of shadow type */
+ skel->struct_ops.testmod_map->data = 13;
+
err = example__load(skel);
if (err)
goto cleanup;
+ /* The result of the function test_2() */
+ printf("test_2_result: %d\n", skel->bss->test_2_result);
+
err = example__attach(skel);
if (err)
goto cleanup;
@@ -372,6 +417,7 @@ BPF ELF object file *example.bpf.o*.
::
+ test_2_result: 17
my_map name: my_map
sys_enter prog FD: 8
my_static_var: 7
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH bpf-next v5 6/6] selftests/bpf: Test if shadow types work correctly.
2024-02-27 1:04 [PATCH bpf-next v5 0/6] Create shadow types for struct_ops maps in skeletons Kui-Feng Lee
` (4 preceding siblings ...)
2024-02-27 1:04 ` [PATCH bpf-next v5 5/6] bpftool: Add an example for struct_ops map and shadow type Kui-Feng Lee
@ 2024-02-27 1:04 ` Kui-Feng Lee
5 siblings, 0 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-27 1:04 UTC (permalink / raw
To: bpf, ast, martin.lau, song, kernel-team, andrii, quentin
Cc: sinquersw, kuifeng, Kui-Feng Lee
Change the values of fields, including scalar types and function pointers,
and check if the struct_ops map works as expected.
The test changes the field "test_2" of "testmod_1" from the pointer to
test_2() to pointer to test_3() and the field "data" to 13. The function
test_2() and test_3() both compute a new value for "test_2_result", but in
different way. By checking the value of "test_2_result", it ensures the
struct_ops map works as expected with changes through shadow types.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 11 ++++++++++-
.../selftests/bpf/bpf_testmod/bpf_testmod.h | 8 ++++++++
.../bpf/prog_tests/test_struct_ops_module.c | 19 +++++++++++++++----
.../selftests/bpf/progs/struct_ops_module.c | 8 ++++++++
4 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 66787e99ba1b..098ddd067224 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -539,6 +539,15 @@ static int bpf_testmod_ops_init_member(const struct btf_type *t,
const struct btf_member *member,
void *kdata, const void *udata)
{
+ if (member->offset == offsetof(struct bpf_testmod_ops, data) * 8) {
+ /* For data fields, this function has to copy it and return
+ * 1 to indicate that the data has been handled by the
+ * struct_ops type, or the verifier will reject the map if
+ * the value of the data field is not zero.
+ */
+ ((struct bpf_testmod_ops *)kdata)->data = ((struct bpf_testmod_ops *)udata)->data;
+ return 1;
+ }
return 0;
}
@@ -559,7 +568,7 @@ static int bpf_dummy_reg(void *kdata)
* initialized, so we need to check for NULL.
*/
if (ops->test_2)
- ops->test_2(4, 3);
+ ops->test_2(4, ops->data);
return 0;
}
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index c3b0cf788f9f..971458acfac3 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
@@ -35,6 +35,14 @@ struct bpf_testmod_ops {
void (*test_2)(int a, int b);
/* Used to test nullable arguments. */
int (*test_maybe_null)(int dummy, struct task_struct *task);
+
+ /* The following fields are used to test shadow copies. */
+ char onebyte;
+ struct {
+ int a;
+ int b;
+ } unsupported;
+ int data;
};
#endif /* _BPF_TESTMOD_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index 8d833f0c7580..7d6facf46ebb 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -32,17 +32,23 @@ static void check_map_info(struct bpf_map_info *info)
static void test_struct_ops_load(void)
{
- DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
struct struct_ops_module *skel;
struct bpf_map_info info = {};
struct bpf_link *link;
int err;
u32 len;
- skel = struct_ops_module__open_opts(&opts);
+ skel = struct_ops_module__open();
if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
return;
+ skel->struct_ops.testmod_1->data = 13;
+ skel->struct_ops.testmod_1->test_2 = skel->progs.test_3;
+ /* Since test_2() is not being used, it should be disabled from
+ * auto-loading, or it will fail to load.
+ */
+ bpf_program__set_autoload(skel->progs.test_2, false);
+
err = struct_ops_module__load(skel);
if (!ASSERT_OK(err, "struct_ops_module_load"))
goto cleanup;
@@ -56,8 +62,13 @@ static void test_struct_ops_load(void)
link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
ASSERT_OK_PTR(link, "attach_test_mod_1");
- /* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
- ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
+ /* test_3() will be called from bpf_dummy_reg() in bpf_testmod.c
+ *
+ * In bpf_testmod.c it will pass 4 and 13 (the value of data) to
+ * .test_2. So, the value of test_2_result should be 20 (4 + 13 +
+ * 3).
+ */
+ ASSERT_EQ(skel->bss->test_2_result, 20, "check_shadow_variables");
bpf_link__destroy(link);
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c
index b78746b3cef3..25952fa09348 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_module.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -21,9 +21,17 @@ void BPF_PROG(test_2, int a, int b)
test_2_result = a + b;
}
+SEC("struct_ops/test_3")
+int BPF_PROG(test_3, int a, int b)
+{
+ test_2_result = a + b + 3;
+ return a + b + 3;
+}
+
SEC(".struct_ops.link")
struct bpf_testmod_ops testmod_1 = {
.test_1 = (void *)test_1,
.test_2 = (void *)test_2,
+ .data = 0x1,
};
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h.
2024-02-27 1:04 ` [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h Kui-Feng Lee
@ 2024-02-28 14:38 ` Quentin Monnet
2024-02-28 17:45 ` Andrii Nakryiko
1 sibling, 0 replies; 24+ messages in thread
From: Quentin Monnet @ 2024-02-28 14:38 UTC (permalink / raw
To: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii
Cc: sinquersw, kuifeng
2024-02-27 01:04 UTC+0000 ~ Kui-Feng Lee <thinker.li@gmail.com>
> bpftool is going to reuse this helper function to support shadow types of
> struct_ops maps.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
Acked-by: Quentin Monnet <quentin@isovalent.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 5/6] bpftool: Add an example for struct_ops map and shadow type.
2024-02-27 1:04 ` [PATCH bpf-next v5 5/6] bpftool: Add an example for struct_ops map and shadow type Kui-Feng Lee
@ 2024-02-28 14:38 ` Quentin Monnet
0 siblings, 0 replies; 24+ messages in thread
From: Quentin Monnet @ 2024-02-28 14:38 UTC (permalink / raw
To: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii
Cc: sinquersw, kuifeng
2024-02-27 01:04 UTC+0000 ~ Kui-Feng Lee <thinker.li@gmail.com>
> 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 <thinker.li@gmail.com>
Thanks for this, and for addressing my previous comments!
Reviewed-by: Quentin Monnet <quentin@isovalent.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h.
2024-02-27 1:04 ` [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h Kui-Feng Lee
2024-02-28 14:38 ` Quentin Monnet
@ 2024-02-28 17:45 ` Andrii Nakryiko
2024-02-28 18:27 ` Kui-Feng Lee
1 sibling, 1 reply; 24+ messages in thread
From: Andrii Nakryiko @ 2024-02-28 17:45 UTC (permalink / raw
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin,
sinquersw, kuifeng
On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> bpftool is going to reuse this helper function to support shadow types of
> struct_ops maps.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 2 +-
> tools/lib/bpf/libbpf_internal.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 01f407591a92..ef8fd20f33ca 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -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;
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index ad936ac5e639..17e6d381da6a 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -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.
>
> static inline enum btf_func_linkage btf_func_linkage(const struct btf_type *t)
> {
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 2/6] libbpf: set btf_value_type_id of struct bpf_map for struct_ops.
2024-02-27 1:04 ` [PATCH bpf-next v5 2/6] libbpf: set btf_value_type_id of struct bpf_map for struct_ops Kui-Feng Lee
@ 2024-02-28 17:48 ` Andrii Nakryiko
2024-02-28 21:24 ` Kui-Feng Lee
0 siblings, 1 reply; 24+ messages in thread
From: Andrii Nakryiko @ 2024-02-28 17:48 UTC (permalink / raw
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin,
sinquersw, kuifeng
On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> 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 <thinker.li@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ef8fd20f33ca..465b50235a01 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -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
> 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 ?
> + 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)
> }
>
> if (bpf_map_type__is_map_in_map(def->type)) {
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type.
2024-02-27 1:04 ` [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type Kui-Feng Lee
@ 2024-02-28 17:58 ` Andrii Nakryiko
2024-02-28 18:18 ` Martin KaFai Lau
2024-02-28 19:27 ` Kui-Feng Lee
0 siblings, 2 replies; 24+ messages in thread
From: Andrii Nakryiko @ 2024-02-28 17:58 UTC (permalink / raw
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin,
sinquersw, kuifeng
On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> 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 <thinker.li@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 465b50235a01..2d22344fb127 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -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.
> 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)
> {
> @@ -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
> + * 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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type.
2024-02-28 17:58 ` Andrii Nakryiko
@ 2024-02-28 18:18 ` Martin KaFai Lau
2024-02-28 19:27 ` Kui-Feng Lee
1 sibling, 0 replies; 24+ messages in thread
From: Martin KaFai Lau @ 2024-02-28 18:18 UTC (permalink / raw
To: Andrii Nakryiko, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, quentin, sinquersw, kuifeng,
Eduard Zingerman
On 2/28/24 9:58 AM, Andrii Nakryiko wrote:
> 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.
+1. The checking that is done at open time (collect_st_ops_relos) should have
been moved here (init_kern_struct_ops, i.e. load time). I saw Eduard (thanks!)
has already done that in his set:
https://lore.kernel.org/bpf/20240227204556.17524-3-eddyz87@gmail.com/
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-27 1:04 ` [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps Kui-Feng Lee
@ 2024-02-28 18:25 ` Andrii Nakryiko
2024-02-28 21:21 ` Kui-Feng Lee
0 siblings, 1 reply; 24+ messages in thread
From: Andrii Nakryiko @ 2024-02-28 18:25 UTC (permalink / raw
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin,
sinquersw, kuifeng
On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> 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 <quentin@isovalent.com>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> tools/bpf/bpftool/gen.c | 235 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 234 insertions(+), 1 deletion(-)
>
Looks pretty good overall, but I have a few stylistical nits, see below.
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index a9334c57e859..a21c92d95401 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -909,6 +909,207 @@ codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li
> }
> }
>
> +static int walk_st_ops_shadow_vars(struct btf *btf,
> + const char *ident,
> + const struct bpf_map *map)
> +{
> + DECLARE_LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts,
> + .indent_level = 3,
> + );
DECLARE_LIBBPF_OPTS is legacy, please use shorter (but equivalent)
LIBBPF_OPTS() macro. Also formatting is a bit weird, let's do just
LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts, .indent_level = 3);
> + 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);
> + if (!map_type)
> + return -EINVAL;
> +
> + d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
> + if (!d)
> + return -errno;
> +
> + for (i = 0, m = btf_members(map_type);
> + i < btf_vlen(map_type);
> + i++, m++) {
let's move `n = btf_vlen(map_type)` out of for loop and keep for()
itself as a single-line
> + member_type = skip_mods_and_typedefs(btf, m->type,
> + &member_type_id);
the line length limit is 100, try to keep single-lines if possible
> + if (!member_type) {
> + err = -EINVAL;
> + goto out;
> + }
this can't happen, no need to add so much error handling
> +
> + member_name = btf__name_by_offset(btf, m->name_off);
> + if (!member_name) {
> + err = -EINVAL;
> + goto out;
> + }
same here, let's assume BTF is correct, no need to double-check these things
> +
> + offset = m->offset / 8;
> + if (next_offset != offset) {
> + printf("\t\t\tchar __padding_%d[%d];\n",
> + i - 1, offset - next_offset);
why i-1? that will give us `__padding_-1[...]` if the very first field
is not aligned (I know, it's unlikely and hypothetical, but still).
Let's just use i as a number for simplicity.
> + }
> +
> + switch (btf_kind(member_type)) {
> + case BTF_KIND_INT:
> + case BTF_KIND_FLOAT:
> + case BTF_KIND_ENUM:
> + case BTF_KIND_ENUM64:
> + /* scalar type */
> + printf("\t\t\t");
> + opts.field_name = member_name;
> + err = btf_dump__emit_type_decl(d, member_type_id,
> + &opts);
same nit about single lines up to 100 characters (I didn't measure if
this fits, but I hope it does)
> + if (err)
> + goto out;
> + printf(";\n");
> +
> + size = btf__resolve_size(btf, member_type_id);
> + if (size < 0) {
> + err = size;
> + goto out;
> + }
> +
> + next_offset = offset + size;
> + break;
> +
> + case BTF_KIND_PTR:
> + if (resolve_func_ptr(btf, m->type, NULL)) {
> + /* Function pointer */
> + printf("\t\t\tconst struct bpf_program *%s;\n",
Why const? technically, user can call libbpf bpf_program___*() APIs on
this and adjust them before the skeleton is loaded, right? Not sure if
const buys us anything, so I'd drop it.
> + 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
> + * in different skeleton files.
> + */
> + if (i == btf_vlen(map_type) - 1) {
> + map_size = btf__resolve_size(btf, map_type_id);
> + if (map_size < 0)
> + return -EINVAL;
> + size = map_size - offset;
> + } else {
> + size = (m[1].offset - m->offset) / 8;
> + }
You are trying to support both unsupported field and any necessary
padding with the same field. I think it's just confuses things. Let's
keep this part just for unsupported field (and thus use field's size),
and then any extra padding between fields or at the end of a struct
should be handled just as pure padding.
> +
> + printf("\t\t\tchar __padding_%d[%d];\n", i, size);
should we name this `__unsupported_%d[%d]` to distinguish from
padding? It would be easier to realize that some portions are actually
not-yet-supported things and call it out, if it should be supported
> +
> + next_offset = offset + size;
> + break;
> + }
> + }
With the above remark about splitting unsupported and padding, you
need to handle remaining padding at the end of a struct here after the
loop
> +
> +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
> + * shadow type. The user can access these members through the pointer.
> + *
> + * A shadow type includes not all members, only members of some types.
> + * They are scalar types and function pointers. The function pointers are
> + * translated to the pointer of the struct bpf_program. The scalar types
> + * are translated to the original type without any modifiers.
> + *
> + * Unsupported types will be translated to a char array to occupy the same
> + * space as the original field. However, the user should not access them
> + * directly. These unsupported fields are also renamed as __padding_*
as mentioned above, I think it's useful to have them named
__unsupported_* so that users can realize that there is something that
is unsupported
> + * . They may be reordered or shifted due to changes in the original struct
nit: dot at the beginning of the line. Also the comment about "may be
reordered or shifted" is confusing, I'm not sure what the problem is.
I'd drop this part of the comment completely, I don't think anything
can be shifted/shuffled, we just don't expose some fields and replace
them with opaque bytes.
> + * 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)
> +{
> + 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.
> +
> + 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.
> + */
> + 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;
> + codegen("\
> + \n\
> + obj->struct_ops.%1$s = \n\
> + bpf_map__initial_value(obj->maps.%1$s, NULL);\n\
nit: keep on single line?
> + \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
> printf("\t\tstruct bpf_map *%s;\n", ident);
> + if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
> + st_ops_cnt++;
> }
> printf("\t} maps;\n");
> + if (st_ops_cnt && btf) {
> + err = gen_st_ops_shadow(btf, obj);
> + if (err)
> + goto out;
> + }
move it out of `if (map_cnt) { ... }` block, just like you do it in
do_subskeleton?
> }
>
> 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)
> @@ -1133,6 +1343,13 @@ static int do_skeleton(int argc, char **argv)
> if (err) \n\
> goto err_out; \n\
> \n\
> + ", obj_name);
> +
> + if (st_ops_cnt && btf)
> + gen_st_ops_shadow_init(btf, obj);
gen_st_ops_shadow_init() and gen_st_ops_shadow() can do st_ops_cnt
calculation inside, keeping this high-level logic a bit cleaner. Just
call `gen_st_ops_shadow_init()` unconditionally and let it do nothing
if there are no struct_ops maps (or no BTF).
> +
> + 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)) {
> @@ -1438,10 +1656,18 @@ static int do_subskeleton(int argc, char **argv)
> if (!get_map_ident(map, ident, sizeof(ident)))
> continue;
> printf("\t\tstruct bpf_map *%s;\n", ident);
> + if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
> + st_ops_cnt++;
see above, I think it can be hidden inside gen_st_ops_shadow[_init]()
functions to keep things cleaner
> }
> 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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h.
2024-02-28 17:45 ` Andrii Nakryiko
@ 2024-02-28 18:27 ` Kui-Feng Lee
0 siblings, 0 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-28 18:27 UTC (permalink / raw
To: Andrii Nakryiko, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin, kuifeng
On 2/28/24 09:45, Andrii Nakryiko wrote:
> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> bpftool is going to reuse this helper function to support shadow types of
>> struct_ops maps.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> tools/lib/bpf/libbpf.c | 2 +-
>> tools/lib/bpf/libbpf_internal.h | 2 ++
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 01f407591a92..ef8fd20f33ca 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -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;
>> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
>> index ad936ac5e639..17e6d381da6a 100644
>> --- a/tools/lib/bpf/libbpf_internal.h
>> +++ b/tools/lib/bpf/libbpf_internal.h
>> @@ -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.
Ok! I will add an implementation in gen.c
>
>>
>> static inline enum btf_func_linkage btf_func_linkage(const struct btf_type *t)
>> {
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type.
2024-02-28 17:58 ` Andrii Nakryiko
2024-02-28 18:18 ` Martin KaFai Lau
@ 2024-02-28 19:27 ` Kui-Feng Lee
1 sibling, 0 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-28 19:27 UTC (permalink / raw
To: Andrii Nakryiko, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin, kuifeng
On 2/28/24 09:58, Andrii Nakryiko wrote:
> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> 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 <thinker.li@gmail.com>
>> ---
>> tools/lib/bpf/libbpf.c | 21 ++++++++++++++++++++-
>> 1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 465b50235a01..2d22344fb127 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -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.
Make sense to me.
>
> 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.
Agree!
Although this will be checked by the kernel, it makes sense to check at
the user space to provide a more meaningful error.
>
>> 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)
>> {
>> @@ -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
>
>> + * 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
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-28 18:25 ` Andrii Nakryiko
@ 2024-02-28 21:21 ` Kui-Feng Lee
2024-02-28 22:28 ` Kui-Feng Lee
0 siblings, 1 reply; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-28 21:21 UTC (permalink / raw
To: Andrii Nakryiko, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin, kuifeng
Will fix most of issues.
On 2/28/24 10:25, Andrii Nakryiko wrote:
> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> + * 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)
>> +{
>> + 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?
>
>> +
>> + err = walk_st_ops_shadow_vars(btf, ident, map);
>> + if (err)
>> + return err;
>> +
>> + printf("\t\t} *%s;\n", ident);
>> +
>> + return 0;
>> +}
>> +
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 2/6] libbpf: set btf_value_type_id of struct bpf_map for struct_ops.
2024-02-28 17:48 ` Andrii Nakryiko
@ 2024-02-28 21:24 ` Kui-Feng Lee
0 siblings, 0 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-28 21:24 UTC (permalink / raw
To: Andrii Nakryiko, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin, kuifeng
On 2/28/24 09:48, Andrii Nakryiko wrote:
> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> 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 <thinker.li@gmail.com>
>> ---
>> tools/lib/bpf/libbpf.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index ef8fd20f33ca..465b50235a01 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -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
>
>> 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 ?
>> + 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)
No problem!
>
>> }
>>
>> if (bpf_map_type__is_map_in_map(def->type)) {
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-28 21:21 ` Kui-Feng Lee
@ 2024-02-28 22:28 ` Kui-Feng Lee
2024-02-29 0:09 ` Andrii Nakryiko
0 siblings, 1 reply; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-28 22:28 UTC (permalink / raw
To: Andrii Nakryiko, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, quentin, kuifeng
On 2/28/24 13:21, Kui-Feng Lee wrote:
> Will fix most of issues.
>
> On 2/28/24 10:25, Andrii Nakryiko wrote:
>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com>
>> wrote:
>>>
>>> + * 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)
>>> +{
>>> + 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?
I misread your words. So, you were saying to prefix the skeleton name,
not map names. It is doable.
>
>>
>>> +
>>> + err = walk_st_ops_shadow_vars(btf, ident, map);
>>> + if (err)
>>> + return err;
>>> +
>>> + printf("\t\t} *%s;\n", ident);
>>> +
>>> + return 0;
>>> +}
>>> +
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-28 22:28 ` Kui-Feng Lee
@ 2024-02-29 0:09 ` Andrii Nakryiko
2024-02-29 0:44 ` Kui-Feng Lee
0 siblings, 1 reply; 24+ messages in thread
From: Andrii Nakryiko @ 2024-02-29 0:09 UTC (permalink / raw
To: Kui-Feng Lee
Cc: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii,
quentin, kuifeng
On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 2/28/24 13:21, Kui-Feng Lee wrote:
> > Will fix most of issues.
> >
> > On 2/28/24 10:25, Andrii Nakryiko wrote:
> >> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com>
> >> wrote:
> >>>
> >>> + * 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)
> >>> +{
> >>> + 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?
>
> 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?
>
> >
> >>
> >>> +
> >>> + err = walk_st_ops_shadow_vars(btf, ident, map);
> >>> + if (err)
> >>> + return err;
> >>> +
> >>> + printf("\t\t} *%s;\n", ident);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-29 0:09 ` Andrii Nakryiko
@ 2024-02-29 0:44 ` Kui-Feng Lee
2024-02-29 0:51 ` Kui-Feng Lee
2024-02-29 1:03 ` Andrii Nakryiko
0 siblings, 2 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-29 0:44 UTC (permalink / raw
To: Andrii Nakryiko
Cc: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii,
quentin, kuifeng
On 2/28/24 16:09, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>>
>> On 2/28/24 13:21, Kui-Feng Lee wrote:
>>> Will fix most of issues.
>>>
>>> On 2/28/24 10:25, Andrii Nakryiko wrote:
>>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com>
>>>> wrote:
>>>>>
>>>>> + * 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)
>>>>> +{
>>>>> + 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?
>>
>> 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?
>
> 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.
>
>>
>>>
>>>>
>>>>> +
>>>>> + err = walk_st_ops_shadow_vars(btf, ident, map);
>>>>> + if (err)
>>>>> + return err;
>>>>> +
>>>>> + printf("\t\t} *%s;\n", ident);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-29 0:44 ` Kui-Feng Lee
@ 2024-02-29 0:51 ` Kui-Feng Lee
2024-02-29 1:03 ` Andrii Nakryiko
1 sibling, 0 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-29 0:51 UTC (permalink / raw
To: Andrii Nakryiko
Cc: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii,
quentin, kuifeng
On 2/28/24 16:44, Kui-Feng Lee wrote:
>
>
> On 2/28/24 16:09, Andrii Nakryiko wrote:
>> On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>>
>>>
>>>
>>> On 2/28/24 13:21, Kui-Feng Lee wrote:
>>>> Will fix most of issues.
>>>>
>>>> On 2/28/24 10:25, Andrii Nakryiko wrote:
>>>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> + * 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)
>>>>>> +{
>>>>>> + 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?
>>>
>>> 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?
Sorry! It should be "<skeleton-type>_bpf_struct_ops_tcp_congestion_ops"
instead of "struct_ops_type".
>
>>
>> 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.
>
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> + err = walk_st_ops_shadow_vars(btf, ident, map);
>>>>>> + if (err)
>>>>>> + return err;
>>>>>> +
>>>>>> + printf("\t\t} *%s;\n", ident);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-29 0:44 ` Kui-Feng Lee
2024-02-29 0:51 ` Kui-Feng Lee
@ 2024-02-29 1:03 ` Andrii Nakryiko
2024-02-29 1:14 ` Kui-Feng Lee
1 sibling, 1 reply; 24+ messages in thread
From: Andrii Nakryiko @ 2024-02-29 1:03 UTC (permalink / raw
To: Kui-Feng Lee
Cc: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii,
quentin, kuifeng
On Wed, Feb 28, 2024 at 4:44 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 2/28/24 16:09, Andrii Nakryiko wrote:
> > On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
> >>
> >>
> >>
> >> On 2/28/24 13:21, Kui-Feng Lee wrote:
> >>> Will fix most of issues.
> >>>
> >>> On 2/28/24 10:25, Andrii Nakryiko wrote:
> >>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com>
> >>>> wrote:
> >>>>>
> >>>>> + * 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)
> >>>>> +{
> >>>>> + 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?
> >>
> >> 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;
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.
> } struct_ops;
> ...
> };
>
> WDYT?
>
> >
> > 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.
>
> >
> >>
> >>>
> >>>>
> >>>>> +
> >>>>> + err = walk_st_ops_shadow_vars(btf, ident, map);
> >>>>> + if (err)
> >>>>> + return err;
> >>>>> +
> >>>>> + printf("\t\t} *%s;\n", ident);
> >>>>> +
> >>>>> + return 0;
> >>>>> +}
> >>>>> +
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.
2024-02-29 1:03 ` Andrii Nakryiko
@ 2024-02-29 1:14 ` Kui-Feng Lee
0 siblings, 0 replies; 24+ messages in thread
From: Kui-Feng Lee @ 2024-02-29 1:14 UTC (permalink / raw
To: Andrii Nakryiko
Cc: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii,
quentin, kuifeng
On 2/28/24 17:03, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 4:44 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>>
>> On 2/28/24 16:09, Andrii Nakryiko wrote:
>>> On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/28/24 13:21, Kui-Feng Lee wrote:
>>>>> Will fix most of issues.
>>>>>
>>>>> On 2/28/24 10:25, Andrii Nakryiko wrote:
>>>>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> + * 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)
>>>>>>> +{
>>>>>>> + 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?
>>>>
>>>> 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;
>
> 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.
Got it
>
>> } struct_ops;
>> ...
>> };
>>
>> WDYT?
>>
>>>
>>> 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.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> + err = walk_st_ops_shadow_vars(btf, ident, map);
>>>>>>> + if (err)
>>>>>>> + return err;
>>>>>>> +
>>>>>>> + printf("\t\t} *%s;\n", ident);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2024-02-29 1:14 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-27 1:04 [PATCH bpf-next v5 0/6] Create shadow types for struct_ops maps in skeletons Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 1/6] libbpf: expose resolve_func_ptr() through libbpf_internal.h Kui-Feng Lee
2024-02-28 14:38 ` Quentin Monnet
2024-02-28 17:45 ` Andrii Nakryiko
2024-02-28 18:27 ` Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 2/6] libbpf: set btf_value_type_id of struct bpf_map for struct_ops Kui-Feng Lee
2024-02-28 17:48 ` Andrii Nakryiko
2024-02-28 21:24 ` Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type Kui-Feng Lee
2024-02-28 17:58 ` Andrii Nakryiko
2024-02-28 18:18 ` Martin KaFai Lau
2024-02-28 19:27 ` Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps Kui-Feng Lee
2024-02-28 18:25 ` Andrii Nakryiko
2024-02-28 21:21 ` Kui-Feng Lee
2024-02-28 22:28 ` Kui-Feng Lee
2024-02-29 0:09 ` Andrii Nakryiko
2024-02-29 0:44 ` Kui-Feng Lee
2024-02-29 0:51 ` Kui-Feng Lee
2024-02-29 1:03 ` Andrii Nakryiko
2024-02-29 1:14 ` Kui-Feng Lee
2024-02-27 1:04 ` [PATCH bpf-next v5 5/6] bpftool: Add an example for struct_ops map and shadow type Kui-Feng Lee
2024-02-28 14:38 ` Quentin Monnet
2024-02-27 1:04 ` [PATCH bpf-next v5 6/6] selftests/bpf: Test if shadow types work correctly Kui-Feng Lee
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.