* [PATCH bpf-next 0/2] Support static initialization of BPF_MAP_TYPE_PROG_ARRAY
@ 2021-11-21 13:54 Hengqi Chen
2021-11-21 13:54 ` [PATCH bpf-next 1/2] libbpf: " Hengqi Chen
2021-11-21 13:54 ` [PATCH bpf-next 2/2] selftests/bpf: Test BPF_MAP_TYPE_PROG_ARRAY static initialization Hengqi Chen
0 siblings, 2 replies; 8+ messages in thread
From: Hengqi Chen @ 2021-11-21 13:54 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, hengqi.chen
Make libbpf support static initialization of BPF_MAP_TYPE_PROG_ARRAY
with a syntax similar to map-in-map initialization:
SEC("socket")
int tailcall_1(void *ctx)
{
return 0;
}
struct {
__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
__uint(max_entries, 2);
__uint(key_size, sizeof(__u32));
__array(values, int (void *));
} prog_array_init SEC(".maps") = {
.values = {
[1] = (void *)&tailcall_1,
},
};
Hengqi Chen (2):
libbpf: Support static initialization of BPF_MAP_TYPE_PROG_ARRAY
selftests/bpf: Test BPF_MAP_TYPE_PROG_ARRAY static initialization
tools/lib/bpf/libbpf.c | 146 +++++++++++++++---
.../bpf/prog_tests/prog_array_init.c | 27 ++++
.../bpf/progs/test_prog_array_init.c | 30 ++++
3 files changed, 179 insertions(+), 24 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/prog_array_init.c
create mode 100644 tools/testing/selftests/bpf/progs/test_prog_array_init.c
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf-next 1/2] libbpf: Support static initialization of BPF_MAP_TYPE_PROG_ARRAY
2021-11-21 13:54 [PATCH bpf-next 0/2] Support static initialization of BPF_MAP_TYPE_PROG_ARRAY Hengqi Chen
@ 2021-11-21 13:54 ` Hengqi Chen
2021-11-23 3:28 ` Andrii Nakryiko
2021-11-21 13:54 ` [PATCH bpf-next 2/2] selftests/bpf: Test BPF_MAP_TYPE_PROG_ARRAY static initialization Hengqi Chen
1 sibling, 1 reply; 8+ messages in thread
From: Hengqi Chen @ 2021-11-21 13:54 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, hengqi.chen
Support static initialization of BPF_MAP_TYPE_PROG_ARRAY with a
syntax similar to map-in-map initialization ([0]):
SEC("socket")
int tailcall_1(void *ctx)
{
return 0;
}
struct {
__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
__uint(max_entries, 2);
__uint(key_size, sizeof(__u32));
__array(values, int (void *));
} prog_array_init SEC(".maps") = {
.values = {
[1] = (void *)&tailcall_1,
},
};
Here's the relevant part of libbpf debug log showing what's
going on with prog-array initialization:
libbpf: sec '.relsocket': collecting relocation for section(3) 'socket'
libbpf: sec '.relsocket': relo #0: insn #2 against 'prog_array_init'
libbpf: prog 'entry': found map 0 (prog_array_init, sec 4, off 0) for insn #0
libbpf: .maps relo #0: for 3 value 0 rel->r_offset 32 name 53 ('tailcall_1')
libbpf: .maps relo #0: map 'prog_array_init' slot [1] points to prog 'tailcall_1'
libbpf: map 'prog_array_init': created successfully, fd=5
libbpf: map 'prog_array_init': slot [1] set to prog 'tailcall_1' fd=6
[0] Closes: https://github.com/libbpf/libbpf/issues/354
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
tools/lib/bpf/libbpf.c | 146 ++++++++++++++++++++++++++++++++++-------
1 file changed, 122 insertions(+), 24 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index af405c38aadc..6b4a175d717d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2277,6 +2277,8 @@ int parse_btf_map_def(const char *map_name, struct btf *btf,
map_def->parts |= MAP_DEF_VALUE_SIZE | MAP_DEF_VALUE_TYPE;
}
else if (strcmp(name, "values") == 0) {
+ bool is_map_in_map = bpf_map_type__is_map_in_map(map_def->map_type);
+ bool is_prog_array = map_def->map_type == BPF_MAP_TYPE_PROG_ARRAY;
char inner_map_name[128];
int err;
@@ -2290,8 +2292,8 @@ int parse_btf_map_def(const char *map_name, struct btf *btf,
map_name, name);
return -EINVAL;
}
- if (!bpf_map_type__is_map_in_map(map_def->map_type)) {
- pr_warn("map '%s': should be map-in-map.\n",
+ if (!is_map_in_map && !is_prog_array) {
+ pr_warn("map '%s': should be map-in-map or prog-array.\n",
map_name);
return -ENOTSUP;
}
@@ -2303,23 +2305,42 @@ int parse_btf_map_def(const char *map_name, struct btf *btf,
map_def->value_size = 4;
t = btf__type_by_id(btf, m->type);
if (!t) {
- pr_warn("map '%s': map-in-map inner type [%d] not found.\n",
- map_name, m->type);
+ if (is_map_in_map)
+ pr_warn("map '%s': map-in-map inner type [%d] not found.\n",
+ map_name, m->type);
+ else
+ pr_warn("map '%s': prog-array value type [%d] not found.\n",
+ map_name, m->type);
return -EINVAL;
}
if (!btf_is_array(t) || btf_array(t)->nelems) {
- pr_warn("map '%s': map-in-map inner spec is not a zero-sized array.\n",
- map_name);
+ if (is_map_in_map)
+ pr_warn("map '%s': map-in-map inner spec is not a zero-sized array.\n",
+ map_name);
+ else
+ pr_warn("map '%s': prog-array value spec is not a zero-sized array.\n",
+ map_name);
return -EINVAL;
}
t = skip_mods_and_typedefs(btf, btf_array(t)->type, NULL);
if (!btf_is_ptr(t)) {
- pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
- map_name, btf_kind_str(t));
+ if (is_map_in_map)
+ pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
+ map_name, btf_kind_str(t));
+ else
+ pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
+ map_name, btf_kind_str(t));
return -EINVAL;
}
t = skip_mods_and_typedefs(btf, t->type, NULL);
- if (!btf_is_struct(t)) {
+ if (is_prog_array) {
+ if (btf_is_func_proto(t))
+ return 0;
+ pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
+ map_name, btf_kind_str(t));
+ return -EINVAL;
+ }
+ if (is_map_in_map && !btf_is_struct(t)) {
pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
map_name, btf_kind_str(t));
return -EINVAL;
@@ -4964,12 +4985,16 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
unsigned int i;
int fd, err = 0;
+ if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY)
+ return 0;
+
for (i = 0; i < map->init_slots_sz; i++) {
if (!map->init_slots[i])
continue;
targ_map = map->init_slots[i];
fd = bpf_map__fd(targ_map);
+
if (obj->gen_loader) {
pr_warn("// TODO map_update_elem: idx %td key %d value==map_idx %td\n",
map - obj->maps, i, targ_map - obj->maps);
@@ -4980,8 +5005,7 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
if (err) {
err = -errno;
pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
- map->name, i, targ_map->name,
- fd, err);
+ map->name, i, targ_map->name, fd, err);
return err;
}
pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
@@ -4994,6 +5018,60 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
return 0;
}
+static int init_prog_array(struct bpf_object *obj, struct bpf_map *map)
+{
+ const struct bpf_program *targ_prog;
+ unsigned int i;
+ int fd, err = 0;
+
+ for (i = 0; i < map->init_slots_sz; i++) {
+ if (!map->init_slots[i])
+ continue;
+
+ targ_prog = map->init_slots[i];
+ fd = bpf_program__fd(targ_prog);
+
+ if (obj->gen_loader) {
+ return -ENOTSUP;
+ } else {
+ err = bpf_map_update_elem(map->fd, &i, &fd, 0);
+ }
+ if (err) {
+ err = -errno;
+ pr_warn("map '%s': failed to initialize slot [%d] to prog '%s' fd=%d: %d\n",
+ map->name, i, targ_prog->name, fd, err);
+ return err;
+ }
+ pr_debug("map '%s': slot [%d] set to prog '%s' fd=%d\n",
+ map->name, i, targ_prog->name, fd);
+ }
+
+ zfree(&map->init_slots);
+ map->init_slots_sz = 0;
+
+ return 0;
+}
+
+static int bpf_object_init_prog_array(struct bpf_object *obj)
+{
+ struct bpf_map *map;
+ int i, err;
+
+ for (i = 0; i < obj->nr_maps; i++) {
+ map = &obj->maps[i];
+
+ if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY &&
+ map->init_slots_sz) {
+ err = init_prog_array(obj, map);
+ if (err < 0) {
+ zclose(map->fd);
+ return err;
+ }
+ }
+ }
+ return 0;
+}
+
static int
bpf_object__create_maps(struct bpf_object *obj)
{
@@ -6174,7 +6252,9 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
int i, j, nrels, new_sz;
const struct btf_var_secinfo *vi = NULL;
const struct btf_type *sec, *var, *def;
- struct bpf_map *map = NULL, *targ_map;
+ struct bpf_map *map = NULL, *targ_map = NULL;
+ struct bpf_program *targ_prog = NULL;
+ bool is_prog_array, is_map_in_map;
const struct btf_member *member;
const char *name, *mname;
unsigned int moff;
@@ -6203,11 +6283,6 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
return -LIBBPF_ERRNO__FORMAT;
}
name = elf_sym_str(obj, sym->st_name) ?: "<?>";
- if (sym->st_shndx != obj->efile.btf_maps_shndx) {
- pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
- i, name);
- return -LIBBPF_ERRNO__RELOC;
- }
pr_debug(".maps relo #%d: for %zd value %zd rel->r_offset %zu name %d ('%s')\n",
i, (ssize_t)(rel->r_info >> 32), (size_t)sym->st_value,
@@ -6229,8 +6304,20 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
return -EINVAL;
}
- if (!bpf_map_type__is_map_in_map(map->def.type))
+ is_map_in_map = bpf_map_type__is_map_in_map(map->def.type);
+ is_prog_array = map->def.type == BPF_MAP_TYPE_PROG_ARRAY;
+ if (!is_map_in_map && !is_prog_array)
return -EINVAL;
+ if (is_map_in_map && sym->st_shndx != obj->efile.btf_maps_shndx) {
+ pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
+ i, name);
+ return -LIBBPF_ERRNO__RELOC;
+ }
+ if (is_prog_array && !bpf_object__find_program_by_name(obj, name)) {
+ pr_warn(".maps relo #%d: '%s' isn't a BPF program\n",
+ i, name);
+ return -LIBBPF_ERRNO__RELOC;
+ }
if (map->def.type == BPF_MAP_TYPE_HASH_OF_MAPS &&
map->def.key_size != sizeof(int)) {
pr_warn(".maps relo #%d: hash-of-maps '%s' should have key size %zu.\n",
@@ -6238,9 +6325,15 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
return -EINVAL;
}
- targ_map = bpf_object__find_map_by_name(obj, name);
- if (!targ_map)
- return -ESRCH;
+ if (is_map_in_map) {
+ targ_map = bpf_object__find_map_by_name(obj, name);
+ if (!targ_map)
+ return -ESRCH;
+ } else {
+ targ_prog = bpf_object__find_program_by_name(obj, name);
+ if (!targ_prog)
+ return -ESRCH;
+ }
var = btf__type_by_id(obj->btf, vi->type);
def = skip_mods_and_typedefs(obj->btf, var->type, NULL);
@@ -6272,10 +6365,14 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
(new_sz - map->init_slots_sz) * host_ptr_sz);
map->init_slots_sz = new_sz;
}
- map->init_slots[moff] = targ_map;
+ map->init_slots[moff] = is_map_in_map ? (void *)targ_map : (void *)targ_prog;
- pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n",
- i, map->name, moff, name);
+ if (is_map_in_map)
+ pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n",
+ i, map->name, moff, name);
+ else
+ pr_debug(".maps relo #%d: map '%s' slot [%d] points to prog '%s'\n",
+ i, map->name, moff, name);
}
return 0;
@@ -7293,6 +7390,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
err = err ? : bpf_object__create_maps(obj);
err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : attr->target_btf_path);
err = err ? : bpf_object__load_progs(obj, attr->log_level);
+ err = err ? : bpf_object_init_prog_array(obj);
if (obj->gen_loader) {
/* reset FDs */
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Test BPF_MAP_TYPE_PROG_ARRAY static initialization
2021-11-21 13:54 [PATCH bpf-next 0/2] Support static initialization of BPF_MAP_TYPE_PROG_ARRAY Hengqi Chen
2021-11-21 13:54 ` [PATCH bpf-next 1/2] libbpf: " Hengqi Chen
@ 2021-11-21 13:54 ` Hengqi Chen
2021-11-23 3:28 ` Andrii Nakryiko
1 sibling, 1 reply; 8+ messages in thread
From: Hengqi Chen @ 2021-11-21 13:54 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, hengqi.chen
Add testcase for BPF_MAP_TYPE_PROG_ARRAY static initialization.
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
.../bpf/prog_tests/prog_array_init.c | 27 +++++++++++++++++
.../bpf/progs/test_prog_array_init.c | 30 +++++++++++++++++++
2 files changed, 57 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/prog_array_init.c
create mode 100644 tools/testing/selftests/bpf/progs/test_prog_array_init.c
diff --git a/tools/testing/selftests/bpf/prog_tests/prog_array_init.c b/tools/testing/selftests/bpf/prog_tests/prog_array_init.c
new file mode 100644
index 000000000000..2fbf6946a0b6
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/prog_array_init.c
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2021 Hengqi Chen */
+
+#include <test_progs.h>
+#include <sys/un.h>
+#include "test_prog_array_init.skel.h"
+
+void test_prog_array_init(void)
+{
+ struct test_prog_array_init *skel;
+ int err;
+
+ skel = test_prog_array_init__open();
+ if (!ASSERT_OK_PTR(skel, "could not open BPF object"))
+ return;
+
+ err = test_prog_array_init__load(skel);
+ if (!ASSERT_OK(err, "could not load BPF object"))
+ goto cleanup;
+
+ err = test_prog_array_init__attach(skel);
+ if (!ASSERT_OK(err, "could not attach BPF object"))
+ goto cleanup;
+
+cleanup:
+ test_prog_array_init__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_prog_array_init.c b/tools/testing/selftests/bpf/progs/test_prog_array_init.c
new file mode 100644
index 000000000000..e97204dd5443
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_prog_array_init.c
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2021 Hengqi Chen */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+SEC("socket")
+int tailcall_1(void *ctx)
+{
+ return 0;
+}
+
+struct {
+ __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+ __uint(max_entries, 2);
+ __uint(key_size, sizeof(__u32));
+ __array(values, int (void *));
+} prog_array_init SEC(".maps") = {
+ .values = {
+ [1] = (void *)&tailcall_1,
+ },
+};
+
+SEC("socket")
+int BPF_PROG(entry)
+{
+ bpf_tail_call(ctx, &prog_array_init, 1);
+ return 0;
+}
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF_MAP_TYPE_PROG_ARRAY static initialization
2021-11-21 13:54 ` [PATCH bpf-next 2/2] selftests/bpf: Test BPF_MAP_TYPE_PROG_ARRAY static initialization Hengqi Chen
@ 2021-11-23 3:28 ` Andrii Nakryiko
2021-11-24 16:18 ` Hengqi Chen
0 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2021-11-23 3:28 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
On Sun, Nov 21, 2021 at 5:55 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Add testcase for BPF_MAP_TYPE_PROG_ARRAY static initialization.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
It's a bit too minimal, let's actually trigger the program and make
sure that tail call program gets executed. Please also make sure that
you filter by pid like other tracing progs do (I suggest using
usleep(1) and raw_tracepoint program for sys_enter, as the simplest
set up).
> .../bpf/prog_tests/prog_array_init.c | 27 +++++++++++++++++
> .../bpf/progs/test_prog_array_init.c | 30 +++++++++++++++++++
> 2 files changed, 57 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/prog_array_init.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_prog_array_init.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/prog_array_init.c b/tools/testing/selftests/bpf/prog_tests/prog_array_init.c
> new file mode 100644
> index 000000000000..2fbf6946a0b6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/prog_array_init.c
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2021 Hengqi Chen */
> +
> +#include <test_progs.h>
> +#include <sys/un.h>
> +#include "test_prog_array_init.skel.h"
> +
> +void test_prog_array_init(void)
> +{
> + struct test_prog_array_init *skel;
> + int err;
> +
> + skel = test_prog_array_init__open();
> + if (!ASSERT_OK_PTR(skel, "could not open BPF object"))
> + return;
> +
> + err = test_prog_array_init__load(skel);
> + if (!ASSERT_OK(err, "could not load BPF object"))
> + goto cleanup;
> +
> + err = test_prog_array_init__attach(skel);
> + if (!ASSERT_OK(err, "could not attach BPF object"))
> + goto cleanup;
> +
> +cleanup:
> + test_prog_array_init__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_prog_array_init.c b/tools/testing/selftests/bpf/progs/test_prog_array_init.c
> new file mode 100644
> index 000000000000..e97204dd5443
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_prog_array_init.c
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2021 Hengqi Chen */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +SEC("socket")
> +int tailcall_1(void *ctx)
> +{
let's add some global variable that will be set by the tail call
program, so that we know that correct slot and correct program was set
> + return 0;
> +}
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> + __uint(max_entries, 2);
> + __uint(key_size, sizeof(__u32));
> + __array(values, int (void *));
> +} prog_array_init SEC(".maps") = {
> + .values = {
> + [1] = (void *)&tailcall_1,
> + },
> +};
> +
> +SEC("socket")
> +int BPF_PROG(entry)
> +{
BPF_PROG doesn't really help, it just hides ctx which you are actually
using below, so let's just stick to `int entry(void *ctx)` here.
> + bpf_tail_call(ctx, &prog_array_init, 1);
> + return 0;
> +}
> --
> 2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] libbpf: Support static initialization of BPF_MAP_TYPE_PROG_ARRAY
2021-11-21 13:54 ` [PATCH bpf-next 1/2] libbpf: " Hengqi Chen
@ 2021-11-23 3:28 ` Andrii Nakryiko
2021-11-24 16:13 ` Hengqi Chen
0 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2021-11-23 3:28 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
On Sun, Nov 21, 2021 at 5:55 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Support static initialization of BPF_MAP_TYPE_PROG_ARRAY with a
> syntax similar to map-in-map initialization ([0]):
>
> SEC("socket")
> int tailcall_1(void *ctx)
> {
> return 0;
> }
>
> struct {
> __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> __uint(max_entries, 2);
> __uint(key_size, sizeof(__u32));
> __array(values, int (void *));
> } prog_array_init SEC(".maps") = {
> .values = {
> [1] = (void *)&tailcall_1,
> },
> };
>
> Here's the relevant part of libbpf debug log showing what's
> going on with prog-array initialization:
>
> libbpf: sec '.relsocket': collecting relocation for section(3) 'socket'
> libbpf: sec '.relsocket': relo #0: insn #2 against 'prog_array_init'
> libbpf: prog 'entry': found map 0 (prog_array_init, sec 4, off 0) for insn #0
> libbpf: .maps relo #0: for 3 value 0 rel->r_offset 32 name 53 ('tailcall_1')
> libbpf: .maps relo #0: map 'prog_array_init' slot [1] points to prog 'tailcall_1'
> libbpf: map 'prog_array_init': created successfully, fd=5
> libbpf: map 'prog_array_init': slot [1] set to prog 'tailcall_1' fd=6
>
> [0] Closes: https://github.com/libbpf/libbpf/issues/354
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 146 ++++++++++++++++++++++++++++++++++-------
> 1 file changed, 122 insertions(+), 24 deletions(-)
>
Just a few nits and suggestions below, but it looks great overall, thanks!
[...]
> t = skip_mods_and_typedefs(btf, btf_array(t)->type, NULL);
> if (!btf_is_ptr(t)) {
> - pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> - map_name, btf_kind_str(t));
> + if (is_map_in_map)
> + pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> + map_name, btf_kind_str(t));
> + else
> + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
maybe let's do
const char *desc = is_map_in_map ? "map-in-map inner" : "prog-array value";
and use desc in those three pr_warn() messages?
> + map_name, btf_kind_str(t));
> return -EINVAL;
> }
> t = skip_mods_and_typedefs(btf, t->type, NULL);
> - if (!btf_is_struct(t)) {
> + if (is_prog_array) {
> + if (btf_is_func_proto(t))
> + return 0;
you can't return on success here, there could technically be other
fields after "values". Can you please also invert the condition so
that error handling happens first and then we continue:
if (!btf_is_func_proto(t)) {
pr_warn(..);
return -EINVAl;
}
continue;
It's more consistent with the other logic in this function
> + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
> + map_name, btf_kind_str(t));
> + return -EINVAL;
> + }
> + if (is_map_in_map && !btf_is_struct(t)) {
well, it can't be anything else, so I guess drop the is_map_in_map check?
> pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> map_name, btf_kind_str(t));
> return -EINVAL;
> @@ -4964,12 +4985,16 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> unsigned int i;
> int fd, err = 0;
>
> + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY)
> + return 0;
let's leave a comment that PROG_ARRAY can only be initialized once all
the programs are loaded, so this will be done later
Better still, it would be good to also rename init_map_slots to
init_map_in_map_slots and do the PROG_ARRAY check outside, in
bpf_object__create_maps(). This creates a nice symmetry with
init_prog_array (should it be named init_prog_array_slots for
consistency?). WDYT?
> +
> for (i = 0; i < map->init_slots_sz; i++) {
> if (!map->init_slots[i])
> continue;
>
> targ_map = map->init_slots[i];
> fd = bpf_map__fd(targ_map);
> +
> if (obj->gen_loader) {
> pr_warn("// TODO map_update_elem: idx %td key %d value==map_idx %td\n",
> map - obj->maps, i, targ_map - obj->maps);
> @@ -4980,8 +5005,7 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> if (err) {
> err = -errno;
> pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
> - map->name, i, targ_map->name,
> - fd, err);
> + map->name, i, targ_map->name, fd, err);
> return err;
> }
> pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
> @@ -4994,6 +5018,60 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> return 0;
> }
>
> +static int init_prog_array(struct bpf_object *obj, struct bpf_map *map)
> +{
> + const struct bpf_program *targ_prog;
> + unsigned int i;
> + int fd, err = 0;
you always set err, no need to zero-initialize it
> +
> + for (i = 0; i < map->init_slots_sz; i++) {
> + if (!map->init_slots[i])
> + continue;
> +
> + targ_prog = map->init_slots[i];
> + fd = bpf_program__fd(targ_prog);
> +
> + if (obj->gen_loader) {
> + return -ENOTSUP;
> + } else {
> + err = bpf_map_update_elem(map->fd, &i, &fd, 0);
> + }
> + if (err) {
> + err = -errno;
> + pr_warn("map '%s': failed to initialize slot [%d] to prog '%s' fd=%d: %d\n",
> + map->name, i, targ_prog->name, fd, err);
> + return err;
> + }
> + pr_debug("map '%s': slot [%d] set to prog '%s' fd=%d\n",
> + map->name, i, targ_prog->name, fd);
> + }
> +
> + zfree(&map->init_slots);
> + map->init_slots_sz = 0;
> +
> + return 0;
> +}
> +
> +static int bpf_object_init_prog_array(struct bpf_object *obj)
s/prog_array/prog_arrays/
> +{
> + struct bpf_map *map;
> + int i, err;
> +
> + for (i = 0; i < obj->nr_maps; i++) {
> + map = &obj->maps[i];
> +
> + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY &&
> + map->init_slots_sz) {
nit: longer single line is fine here (but you can also invert the
condition and continue early to avoid extra level of nestedness)
> + err = init_prog_array(obj, map);
> + if (err < 0) {
> + zclose(map->fd);
> + return err;
> + }
> + }
> + }
> + return 0;
> +}
> +
> static int
> bpf_object__create_maps(struct bpf_object *obj)
> {
> @@ -6174,7 +6252,9 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> int i, j, nrels, new_sz;
> const struct btf_var_secinfo *vi = NULL;
> const struct btf_type *sec, *var, *def;
> - struct bpf_map *map = NULL, *targ_map;
> + struct bpf_map *map = NULL, *targ_map = NULL;
> + struct bpf_program *targ_prog = NULL;
> + bool is_prog_array, is_map_in_map;
> const struct btf_member *member;
> const char *name, *mname;
> unsigned int moff;
> @@ -6203,11 +6283,6 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> return -LIBBPF_ERRNO__FORMAT;
> }
> name = elf_sym_str(obj, sym->st_name) ?: "<?>";
> - if (sym->st_shndx != obj->efile.btf_maps_shndx) {
> - pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
> - i, name);
> - return -LIBBPF_ERRNO__RELOC;
> - }
>
> pr_debug(".maps relo #%d: for %zd value %zd rel->r_offset %zu name %d ('%s')\n",
> i, (ssize_t)(rel->r_info >> 32), (size_t)sym->st_value,
> @@ -6229,8 +6304,20 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> return -EINVAL;
> }
>
> - if (!bpf_map_type__is_map_in_map(map->def.type))
> + is_map_in_map = bpf_map_type__is_map_in_map(map->def.type);
> + is_prog_array = map->def.type == BPF_MAP_TYPE_PROG_ARRAY;
> + if (!is_map_in_map && !is_prog_array)
> return -EINVAL;
> + if (is_map_in_map && sym->st_shndx != obj->efile.btf_maps_shndx) {
> + pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
> + i, name);
> + return -LIBBPF_ERRNO__RELOC;
> + }
> + if (is_prog_array && !bpf_object__find_program_by_name(obj, name)) {
let's do an additional check on the program you found with find_program_by_name:
1. prog->sec_idx == sym->st_shndx
2. prog->sec_insn_off * 8 == sym->st_value
3. !prog_is_subprog(obj, prog)
This will make sure you have the correct entry-point BPF program (not
a subprog) and we point to its beginning (no offset into the program).
> + pr_warn(".maps relo #%d: '%s' isn't a BPF program\n",
"entry-point" is an important distinction, please mention that. You
can't put sub-programs into PROG_ARRAY.
> + i, name);
> + return -LIBBPF_ERRNO__RELOC;
> + }
> if (map->def.type == BPF_MAP_TYPE_HASH_OF_MAPS &&
> map->def.key_size != sizeof(int)) {
> pr_warn(".maps relo #%d: hash-of-maps '%s' should have key size %zu.\n",
> @@ -6238,9 +6325,15 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> return -EINVAL;
> }
>
> - targ_map = bpf_object__find_map_by_name(obj, name);
> - if (!targ_map)
> - return -ESRCH;
> + if (is_map_in_map) {
> + targ_map = bpf_object__find_map_by_name(obj, name);
> + if (!targ_map)
> + return -ESRCH;
> + } else {
> + targ_prog = bpf_object__find_program_by_name(obj, name);
> + if (!targ_prog)
> + return -ESRCH;
> + }
>
> var = btf__type_by_id(obj->btf, vi->type);
> def = skip_mods_and_typedefs(obj->btf, var->type, NULL);
> @@ -6272,10 +6365,14 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> (new_sz - map->init_slots_sz) * host_ptr_sz);
> map->init_slots_sz = new_sz;
> }
> - map->init_slots[moff] = targ_map;
> + map->init_slots[moff] = is_map_in_map ? (void *)targ_map : (void *)targ_prog;
>
> - pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n",
> - i, map->name, moff, name);
> + if (is_map_in_map)
> + pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n",
> + i, map->name, moff, name);
> + else
> + pr_debug(".maps relo #%d: map '%s' slot [%d] points to prog '%s'\n",
> + i, map->name, moff, name);
similar as above `is_map_in_map ? "map" : "prog"` will keep it short
and not duplicated
> }
>
> return 0;
> @@ -7293,6 +7390,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> err = err ? : bpf_object__create_maps(obj);
> err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : attr->target_btf_path);
> err = err ? : bpf_object__load_progs(obj, attr->log_level);
> + err = err ? : bpf_object_init_prog_array(obj);
>
> if (obj->gen_loader) {
> /* reset FDs */
> --
> 2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] libbpf: Support static initialization of BPF_MAP_TYPE_PROG_ARRAY
2021-11-23 3:28 ` Andrii Nakryiko
@ 2021-11-24 16:13 ` Hengqi Chen
2021-11-24 19:16 ` Andrii Nakryiko
0 siblings, 1 reply; 8+ messages in thread
From: Hengqi Chen @ 2021-11-24 16:13 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Hi, Andrii
On 11/23/21 11:28 AM, Andrii Nakryiko wrote:
> On Sun, Nov 21, 2021 at 5:55 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>
>> Support static initialization of BPF_MAP_TYPE_PROG_ARRAY with a
>> syntax similar to map-in-map initialization ([0]):
>>
>> SEC("socket")
>> int tailcall_1(void *ctx)
>> {
>> return 0;
>> }
>>
>> struct {
>> __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>> __uint(max_entries, 2);
>> __uint(key_size, sizeof(__u32));
>> __array(values, int (void *));
>> } prog_array_init SEC(".maps") = {
>> .values = {
>> [1] = (void *)&tailcall_1,
>> },
>> };
>>
>> Here's the relevant part of libbpf debug log showing what's
>> going on with prog-array initialization:
>>
>> libbpf: sec '.relsocket': collecting relocation for section(3) 'socket'
>> libbpf: sec '.relsocket': relo #0: insn #2 against 'prog_array_init'
>> libbpf: prog 'entry': found map 0 (prog_array_init, sec 4, off 0) for insn #0
>> libbpf: .maps relo #0: for 3 value 0 rel->r_offset 32 name 53 ('tailcall_1')
>> libbpf: .maps relo #0: map 'prog_array_init' slot [1] points to prog 'tailcall_1'
>> libbpf: map 'prog_array_init': created successfully, fd=5
>> libbpf: map 'prog_array_init': slot [1] set to prog 'tailcall_1' fd=6
>>
>> [0] Closes: https://github.com/libbpf/libbpf/issues/354
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>> tools/lib/bpf/libbpf.c | 146 ++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 122 insertions(+), 24 deletions(-)
>>
>
> Just a few nits and suggestions below, but it looks great overall, thanks!
>
> [...]
>
>> t = skip_mods_and_typedefs(btf, btf_array(t)->type, NULL);
>> if (!btf_is_ptr(t)) {
>> - pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
>> - map_name, btf_kind_str(t));
>> + if (is_map_in_map)
>> + pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
>> + map_name, btf_kind_str(t));
>> + else
>> + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
>
> maybe let's do
>
> const char *desc = is_map_in_map ? "map-in-map inner" : "prog-array value";
>
> and use desc in those three pr_warn() messages?
>
Ack.
>> + map_name, btf_kind_str(t));
>> return -EINVAL;
>> }
>> t = skip_mods_and_typedefs(btf, t->type, NULL);
>> - if (!btf_is_struct(t)) {
>> + if (is_prog_array) {
>> + if (btf_is_func_proto(t))
>> + return 0;
>
> you can't return on success here, there could technically be other
> fields after "values". Can you please also invert the condition so
> that error handling happens first and then we continue:
>
According to the original code ([0]), the "values" field is intended to be
the last field ?
> if (!btf_is_func_proto(t)) {
> pr_warn(..);
> return -EINVAl;
> }
> continue;
>
> It's more consistent with the other logic in this function
>
>
>> + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
>> + map_name, btf_kind_str(t));
>> + return -EINVAL;
>> + }
>> + if (is_map_in_map && !btf_is_struct(t)) {
>
> well, it can't be anything else, so I guess drop the is_map_in_map check?
>
Right, will do.
>> pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
>> map_name, btf_kind_str(t));
>> return -EINVAL;
>> @@ -4964,12 +4985,16 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
>> unsigned int i;
>> int fd, err = 0;
>>
>> + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY)
>> + return 0;
>
> let's leave a comment that PROG_ARRAY can only be initialized once all
> the programs are loaded, so this will be done later
>
> Better still, it would be good to also rename init_map_slots to
> init_map_in_map_slots and do the PROG_ARRAY check outside, in
> bpf_object__create_maps(). This creates a nice symmetry with
> init_prog_array (should it be named init_prog_array_slots for
> consistency?). WDYT?
>
Ack.
>> +
>> for (i = 0; i < map->init_slots_sz; i++) {
>> if (!map->init_slots[i])
>> continue;
>>
>> targ_map = map->init_slots[i];
>> fd = bpf_map__fd(targ_map);
>> +
>> if (obj->gen_loader) {
>> pr_warn("// TODO map_update_elem: idx %td key %d value==map_idx %td\n",
>> map - obj->maps, i, targ_map - obj->maps);
>> @@ -4980,8 +5005,7 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
>> if (err) {
>> err = -errno;
>> pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
>> - map->name, i, targ_map->name,
>> - fd, err);
>> + map->name, i, targ_map->name, fd, err);
>> return err;
>> }
>> pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
>> @@ -4994,6 +5018,60 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
>> return 0;
>> }
>>
>> +static int init_prog_array(struct bpf_object *obj, struct bpf_map *map)
>> +{
>> + const struct bpf_program *targ_prog;
>> + unsigned int i;
>> + int fd, err = 0;
>
> you always set err, no need to zero-initialize it
>
OK, will remove it.
>> +
>> + for (i = 0; i < map->init_slots_sz; i++) {
>> + if (!map->init_slots[i])
>> + continue;
>> +
>> + targ_prog = map->init_slots[i];
>> + fd = bpf_program__fd(targ_prog);
>> +
>> + if (obj->gen_loader) {
>> + return -ENOTSUP;
>> + } else {
>> + err = bpf_map_update_elem(map->fd, &i, &fd, 0);
>> + }
>> + if (err) {
>> + err = -errno;
>> + pr_warn("map '%s': failed to initialize slot [%d] to prog '%s' fd=%d: %d\n",
>> + map->name, i, targ_prog->name, fd, err);
>> + return err;
>> + }
>> + pr_debug("map '%s': slot [%d] set to prog '%s' fd=%d\n",
>> + map->name, i, targ_prog->name, fd);
>> + }
>> +
>> + zfree(&map->init_slots);
>> + map->init_slots_sz = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static int bpf_object_init_prog_array(struct bpf_object *obj)
>
> s/prog_array/prog_arrays/
>
>> +{
>> + struct bpf_map *map;
>> + int i, err;
>> +
>> + for (i = 0; i < obj->nr_maps; i++) {
>> + map = &obj->maps[i];
>> +
>> + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY &&
>> + map->init_slots_sz) {
>
> nit: longer single line is fine here (but you can also invert the
> condition and continue early to avoid extra level of nestedness)
>
Will do.
>> + err = init_prog_array(obj, map);
>> + if (err < 0) {
>> + zclose(map->fd);
>> + return err;
>> + }
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> static int
>> bpf_object__create_maps(struct bpf_object *obj)
>> {
>> @@ -6174,7 +6252,9 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
>> int i, j, nrels, new_sz;
>> const struct btf_var_secinfo *vi = NULL;
>> const struct btf_type *sec, *var, *def;
>> - struct bpf_map *map = NULL, *targ_map;
>> + struct bpf_map *map = NULL, *targ_map = NULL;
>> + struct bpf_program *targ_prog = NULL;
>> + bool is_prog_array, is_map_in_map;
>> const struct btf_member *member;
>> const char *name, *mname;
>> unsigned int moff;
>> @@ -6203,11 +6283,6 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
>> return -LIBBPF_ERRNO__FORMAT;
>> }
>> name = elf_sym_str(obj, sym->st_name) ?: "<?>";
>> - if (sym->st_shndx != obj->efile.btf_maps_shndx) {
>> - pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
>> - i, name);
>> - return -LIBBPF_ERRNO__RELOC;
>> - }
>>
>> pr_debug(".maps relo #%d: for %zd value %zd rel->r_offset %zu name %d ('%s')\n",
>> i, (ssize_t)(rel->r_info >> 32), (size_t)sym->st_value,
>> @@ -6229,8 +6304,20 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
>> return -EINVAL;
>> }
>>
>> - if (!bpf_map_type__is_map_in_map(map->def.type))
>> + is_map_in_map = bpf_map_type__is_map_in_map(map->def.type);
>> + is_prog_array = map->def.type == BPF_MAP_TYPE_PROG_ARRAY;
>> + if (!is_map_in_map && !is_prog_array)
>> return -EINVAL;
>> + if (is_map_in_map && sym->st_shndx != obj->efile.btf_maps_shndx) {
>> + pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
>> + i, name);
>> + return -LIBBPF_ERRNO__RELOC;
>> + }
>> + if (is_prog_array && !bpf_object__find_program_by_name(obj, name)) {
>
> let's do an additional check on the program you found with find_program_by_name:
>
> 1. prog->sec_idx == sym->st_shndx
> 2. prog->sec_insn_off * 8 == sym->st_value
> 3. !prog_is_subprog(obj, prog)
>
> This will make sure you have the correct entry-point BPF program (not
> a subprog) and we point to its beginning (no offset into the program).
>
>
OK, will do, maybe we should also add some tests for these cases ?
>> + pr_warn(".maps relo #%d: '%s' isn't a BPF program\n",
>
> "entry-point" is an important distinction, please mention that. You
> can't put sub-programs into PROG_ARRAY.
>
>> + i, name);
>> + return -LIBBPF_ERRNO__RELOC;
>> + }
>> if (map->def.type == BPF_MAP_TYPE_HASH_OF_MAPS &&
>> map->def.key_size != sizeof(int)) {
>> pr_warn(".maps relo #%d: hash-of-maps '%s' should have key size %zu.\n",
>> @@ -6238,9 +6325,15 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
>> return -EINVAL;
>> }
>>
>> - targ_map = bpf_object__find_map_by_name(obj, name);
>> - if (!targ_map)
>> - return -ESRCH;
>> + if (is_map_in_map) {
>> + targ_map = bpf_object__find_map_by_name(obj, name);
>> + if (!targ_map)
>> + return -ESRCH;
>> + } else {
>> + targ_prog = bpf_object__find_program_by_name(obj, name);
>> + if (!targ_prog)
>> + return -ESRCH;
>> + }
>>
>> var = btf__type_by_id(obj->btf, vi->type);
>> def = skip_mods_and_typedefs(obj->btf, var->type, NULL);
>> @@ -6272,10 +6365,14 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
>> (new_sz - map->init_slots_sz) * host_ptr_sz);
>> map->init_slots_sz = new_sz;
>> }
>> - map->init_slots[moff] = targ_map;
>> + map->init_slots[moff] = is_map_in_map ? (void *)targ_map : (void *)targ_prog;
>>
>> - pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n",
>> - i, map->name, moff, name);
>> + if (is_map_in_map)
>> + pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n",
>> + i, map->name, moff, name);
>> + else
>> + pr_debug(".maps relo #%d: map '%s' slot [%d] points to prog '%s'\n",
>> + i, map->name, moff, name);
>
> similar as above `is_map_in_map ? "map" : "prog"` will keep it short
> and not duplicated
>
>
Ack.
>> }
>>
>> return 0;
>> @@ -7293,6 +7390,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>> err = err ? : bpf_object__create_maps(obj);
>> err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : attr->target_btf_path);
>> err = err ? : bpf_object__load_progs(obj, attr->log_level);
>> + err = err ? : bpf_object_init_prog_array(obj);
>>
>> if (obj->gen_loader) {
>> /* reset FDs */
>> --
>> 2.30.2
[0]: https://github.com/libbpf/libbpf/blob/master/src/libbpf.c#L2288-L2292
Cheers,
---
Hengqi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF_MAP_TYPE_PROG_ARRAY static initialization
2021-11-23 3:28 ` Andrii Nakryiko
@ 2021-11-24 16:18 ` Hengqi Chen
0 siblings, 0 replies; 8+ messages in thread
From: Hengqi Chen @ 2021-11-24 16:18 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
On 11/23/21 11:28 AM, Andrii Nakryiko wrote:
> On Sun, Nov 21, 2021 at 5:55 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>
>> Add testcase for BPF_MAP_TYPE_PROG_ARRAY static initialization.
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>
> It's a bit too minimal, let's actually trigger the program and make
> sure that tail call program gets executed. Please also make sure that
> you filter by pid like other tracing progs do (I suggest using
> usleep(1) and raw_tracepoint program for sys_enter, as the simplest
> set up).
>
Will implement what you suggest here. Thanks.
>> .../bpf/prog_tests/prog_array_init.c | 27 +++++++++++++++++
>> .../bpf/progs/test_prog_array_init.c | 30 +++++++++++++++++++
>> 2 files changed, 57 insertions(+)
>> create mode 100644 tools/testing/selftests/bpf/prog_tests/prog_array_init.c
>> create mode 100644 tools/testing/selftests/bpf/progs/test_prog_array_init.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/prog_array_init.c b/tools/testing/selftests/bpf/prog_tests/prog_array_init.c
>> new file mode 100644
>> index 000000000000..2fbf6946a0b6
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/prog_array_init.c
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2021 Hengqi Chen */
>> +
>> +#include <test_progs.h>
>> +#include <sys/un.h>
>> +#include "test_prog_array_init.skel.h"
>> +
>> +void test_prog_array_init(void)
>> +{
>> + struct test_prog_array_init *skel;
>> + int err;
>> +
>> + skel = test_prog_array_init__open();
>> + if (!ASSERT_OK_PTR(skel, "could not open BPF object"))
>> + return;
>> +
>> + err = test_prog_array_init__load(skel);
>> + if (!ASSERT_OK(err, "could not load BPF object"))
>> + goto cleanup;
>> +
>> + err = test_prog_array_init__attach(skel);
>> + if (!ASSERT_OK(err, "could not attach BPF object"))
>> + goto cleanup;
>> +
>> +cleanup:
>> + test_prog_array_init__destroy(skel);
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/test_prog_array_init.c b/tools/testing/selftests/bpf/progs/test_prog_array_init.c
>> new file mode 100644
>> index 000000000000..e97204dd5443
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/test_prog_array_init.c
>> @@ -0,0 +1,30 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2021 Hengqi Chen */
>> +
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +SEC("socket")
>> +int tailcall_1(void *ctx)
>> +{
>
> let's add some global variable that will be set by the tail call
> program, so that we know that correct slot and correct program was set
>
Will do.
>> + return 0;
>> +}
>> +
>> +struct {
>> + __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>> + __uint(max_entries, 2);
>> + __uint(key_size, sizeof(__u32));
>> + __array(values, int (void *));
>> +} prog_array_init SEC(".maps") = {
>> + .values = {
>> + [1] = (void *)&tailcall_1,
>> + },
>> +};
>> +
>> +SEC("socket")
>> +int BPF_PROG(entry)
>> +{
>
> BPF_PROG doesn't really help, it just hides ctx which you are actually
> using below, so let's just stick to `int entry(void *ctx)` here.
>
Ack.
>> + bpf_tail_call(ctx, &prog_array_init, 1);
>> + return 0;
>> +}
>> --
>> 2.30.2
---Hengqi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] libbpf: Support static initialization of BPF_MAP_TYPE_PROG_ARRAY
2021-11-24 16:13 ` Hengqi Chen
@ 2021-11-24 19:16 ` Andrii Nakryiko
0 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2021-11-24 19:16 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
On Wed, Nov 24, 2021 at 8:13 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Hi, Andrii
>
> On 11/23/21 11:28 AM, Andrii Nakryiko wrote:
> > On Sun, Nov 21, 2021 at 5:55 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
> >>
> >> Support static initialization of BPF_MAP_TYPE_PROG_ARRAY with a
> >> syntax similar to map-in-map initialization ([0]):
> >>
> >> SEC("socket")
> >> int tailcall_1(void *ctx)
> >> {
> >> return 0;
> >> }
> >>
> >> struct {
> >> __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> >> __uint(max_entries, 2);
> >> __uint(key_size, sizeof(__u32));
> >> __array(values, int (void *));
> >> } prog_array_init SEC(".maps") = {
> >> .values = {
> >> [1] = (void *)&tailcall_1,
> >> },
> >> };
> >>
> >> Here's the relevant part of libbpf debug log showing what's
> >> going on with prog-array initialization:
> >>
> >> libbpf: sec '.relsocket': collecting relocation for section(3) 'socket'
> >> libbpf: sec '.relsocket': relo #0: insn #2 against 'prog_array_init'
> >> libbpf: prog 'entry': found map 0 (prog_array_init, sec 4, off 0) for insn #0
> >> libbpf: .maps relo #0: for 3 value 0 rel->r_offset 32 name 53 ('tailcall_1')
> >> libbpf: .maps relo #0: map 'prog_array_init' slot [1] points to prog 'tailcall_1'
> >> libbpf: map 'prog_array_init': created successfully, fd=5
> >> libbpf: map 'prog_array_init': slot [1] set to prog 'tailcall_1' fd=6
> >>
> >> [0] Closes: https://github.com/libbpf/libbpf/issues/354
> >>
> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> >> ---
> >> tools/lib/bpf/libbpf.c | 146 ++++++++++++++++++++++++++++++++++-------
> >> 1 file changed, 122 insertions(+), 24 deletions(-)
> >>
> >
> > Just a few nits and suggestions below, but it looks great overall, thanks!
> >
> > [...]
> >
> >> t = skip_mods_and_typedefs(btf, btf_array(t)->type, NULL);
> >> if (!btf_is_ptr(t)) {
> >> - pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> >> - map_name, btf_kind_str(t));
> >> + if (is_map_in_map)
> >> + pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> >> + map_name, btf_kind_str(t));
> >> + else
> >> + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
> >
> > maybe let's do
> >
> > const char *desc = is_map_in_map ? "map-in-map inner" : "prog-array value";
> >
> > and use desc in those three pr_warn() messages?
> >
>
> Ack.
>
> >> + map_name, btf_kind_str(t));
> >> return -EINVAL;
> >> }
> >> t = skip_mods_and_typedefs(btf, t->type, NULL);
> >> - if (!btf_is_struct(t)) {
> >> + if (is_prog_array) {
> >> + if (btf_is_func_proto(t))
> >> + return 0;
> >
> > you can't return on success here, there could technically be other
> > fields after "values". Can you please also invert the condition so
> > that error handling happens first and then we continue:
> >
> According to the original code ([0]), the "values" field is intended to be
>
> the last field ?
yeah, but we don't need to make this assumption here and exit early,
given the other code doesn't do that. Let's not try to shoot ourselves
in the foot unnecessarily.
>
> > if (!btf_is_func_proto(t)) {
> > pr_warn(..);
> > return -EINVAl;
> > }
> > continue;
> >
> > It's more consistent with the other logic in this function
> >
> >
[...]
> >> @@ -6229,8 +6304,20 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> >> return -EINVAL;
> >> }
> >>
> >> - if (!bpf_map_type__is_map_in_map(map->def.type))
> >> + is_map_in_map = bpf_map_type__is_map_in_map(map->def.type);
> >> + is_prog_array = map->def.type == BPF_MAP_TYPE_PROG_ARRAY;
> >> + if (!is_map_in_map && !is_prog_array)
> >> return -EINVAL;
> >> + if (is_map_in_map && sym->st_shndx != obj->efile.btf_maps_shndx) {
> >> + pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
> >> + i, name);
> >> + return -LIBBPF_ERRNO__RELOC;
> >> + }
> >> + if (is_prog_array && !bpf_object__find_program_by_name(obj, name)) {
> >
> > let's do an additional check on the program you found with find_program_by_name:
> >
> > 1. prog->sec_idx == sym->st_shndx
> > 2. prog->sec_insn_off * 8 == sym->st_value
> > 3. !prog_is_subprog(obj, prog)
> >
> > This will make sure you have the correct entry-point BPF program (not
> > a subprog) and we point to its beginning (no offset into the program).
> >
> >
>
> OK, will do, maybe we should also add some tests for these cases ?
yeah, negative test validating that you can't put a global variable
and/or a map into the PROG_ARRAY slot would be great, thanks!
>
> >> + pr_warn(".maps relo #%d: '%s' isn't a BPF program\n",
> >
> > "entry-point" is an important distinction, please mention that. You
> > can't put sub-programs into PROG_ARRAY.
> >
> >> + i, name);
> >> + return -LIBBPF_ERRNO__RELOC;
> >> + }
> >> if (map->def.type == BPF_MAP_TYPE_HASH_OF_MAPS &&
> >> map->def.key_size != sizeof(int)) {
> >> pr_warn(".maps relo #%d: hash-of-maps '%s' should have key size %zu.\n",
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-11-24 19:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-21 13:54 [PATCH bpf-next 0/2] Support static initialization of BPF_MAP_TYPE_PROG_ARRAY Hengqi Chen
2021-11-21 13:54 ` [PATCH bpf-next 1/2] libbpf: " Hengqi Chen
2021-11-23 3:28 ` Andrii Nakryiko
2021-11-24 16:13 ` Hengqi Chen
2021-11-24 19:16 ` Andrii Nakryiko
2021-11-21 13:54 ` [PATCH bpf-next 2/2] selftests/bpf: Test BPF_MAP_TYPE_PROG_ARRAY static initialization Hengqi Chen
2021-11-23 3:28 ` Andrii Nakryiko
2021-11-24 16:18 ` Hengqi Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).