* [PATCH bpf-next v5 1/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME
2021-10-21 21:48 [PATCH bpf-next v5 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
@ 2021-10-21 21:48 ` Stanislav Fomichev
2021-10-21 21:48 ` [PATCH bpf-next v5 2/3] bpftool: conditionally append / to the progtype Stanislav Fomichev
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Stanislav Fomichev @ 2021-10-21 21:48 UTC (permalink / raw)
To: netdev, bpf; +Cc: ast, daniel, andrii, Stanislav Fomichev, Quentin Monnet
We can't use section name anymore because they are not unique
and pinning objects with multiple programs with the same
progtype/secname will fail.
Closes: https://github.com/libbpf/libbpf/issues/273
Fixes: 33a2c75c55e2 ("libbpf: add internal pin_name")
Reviewed-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/lib/bpf/libbpf.c | 13 +++++++++++--
tools/lib/bpf/libbpf_legacy.h | 3 +++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 760c7e346603..7f48eeb3ca82 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -285,7 +285,7 @@ struct bpf_program {
size_t sub_insn_off;
char *name;
- /* sec_name with / replaced by _; makes recursive pinning
+ /* name with / replaced by _; makes recursive pinning
* in bpf_object__pin_programs easier
*/
char *pin_name;
@@ -614,7 +614,16 @@ static char *__bpf_program__pin_name(struct bpf_program *prog)
{
char *name, *p;
- name = p = strdup(prog->sec_name);
+ if (libbpf_mode & LIBBPF_STRICT_SEC_NAME)
+ name = strdup(prog->name);
+ else
+ name = strdup(prog->sec_name);
+
+ if (!name)
+ return NULL;
+
+ p = name;
+
while ((p = strchr(p, '/')))
*p = '_';
diff --git a/tools/lib/bpf/libbpf_legacy.h b/tools/lib/bpf/libbpf_legacy.h
index 74e6f860f703..29ccafab11a8 100644
--- a/tools/lib/bpf/libbpf_legacy.h
+++ b/tools/lib/bpf/libbpf_legacy.h
@@ -52,6 +52,9 @@ enum libbpf_strict_mode {
* allowed, with LIBBPF_STRICT_SEC_PREFIX this will become
* unrecognized by libbpf and would have to be just SEC("xdp") and
* SEC("xdp") and SEC("perf_event").
+ *
+ * Note, in this mode the program pin path will be based on the
+ * function name instead of section name.
*/
LIBBPF_STRICT_SEC_NAME = 0x04,
--
2.33.0.1079.g6e70778dc9-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next v5 2/3] bpftool: conditionally append / to the progtype
2021-10-21 21:48 [PATCH bpf-next v5 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
2021-10-21 21:48 ` [PATCH bpf-next v5 1/3] " Stanislav Fomichev
@ 2021-10-21 21:48 ` Stanislav Fomichev
2021-10-22 8:20 ` Quentin Monnet
2021-10-21 21:48 ` [PATCH bpf-next v5 3/3] selftests/bpf: fix flow dissector tests Stanislav Fomichev
2021-10-22 23:57 ` [PATCH bpf-next v5 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Andrii Nakryiko
3 siblings, 1 reply; 6+ messages in thread
From: Stanislav Fomichev @ 2021-10-21 21:48 UTC (permalink / raw)
To: netdev, bpf; +Cc: ast, daniel, andrii, Stanislav Fomichev, Quentin Monnet
Otherwise, attaching with bpftool doesn't work with strict section names.
Also, switch to libbpf strict mode to use the latest conventions
(note, I don't think we have any cli api guarantees?).
Cc: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/bpf/bpftool/main.c | 4 ++++
tools/bpf/bpftool/prog.c | 35 +++++++++++++++++++----------------
2 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 02eaaf065f65..8223bac1e401 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -409,6 +409,10 @@ int main(int argc, char **argv)
block_mount = false;
bin_name = argv[0];
+ ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
+ if (ret)
+ p_err("failed to enable libbpf strict mode: %d", ret);
+
hash_init(prog_table.table);
hash_init(map_table.table);
hash_init(link_table.table);
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 277d51c4c5d9..6fec425d5390 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
while (argc) {
if (is_prefix(*argv, "type")) {
- char *type;
-
NEXT_ARG();
if (common_prog_type != BPF_PROG_TYPE_UNSPEC) {
@@ -1407,21 +1405,26 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
if (!REQ_ARGS(1))
goto err_free_reuse_maps;
- /* Put a '/' at the end of type to appease libbpf */
- type = malloc(strlen(*argv) + 2);
- if (!type) {
- p_err("mem alloc failed");
- goto err_free_reuse_maps;
- }
- *type = 0;
- strcat(type, *argv);
- strcat(type, "/");
+ err = libbpf_prog_type_by_name(*argv, &common_prog_type,
+ &expected_attach_type);
+ if (err < 0) {
+ /* Put a '/' at the end of type to appease libbpf */
+ char *type = malloc(strlen(*argv) + 2);
- err = get_prog_type_by_name(type, &common_prog_type,
- &expected_attach_type);
- free(type);
- if (err < 0)
- goto err_free_reuse_maps;
+ if (!type) {
+ p_err("mem alloc failed");
+ goto err_free_reuse_maps;
+ }
+ *type = 0;
+ strcat(type, *argv);
+ strcat(type, "/");
+
+ err = get_prog_type_by_name(type, &common_prog_type,
+ &expected_attach_type);
+ free(type);
+ if (err < 0)
+ goto err_free_reuse_maps;
+ }
NEXT_ARG();
} else if (is_prefix(*argv, "map")) {
--
2.33.0.1079.g6e70778dc9-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next v5 3/3] selftests/bpf: fix flow dissector tests
2021-10-21 21:48 [PATCH bpf-next v5 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
2021-10-21 21:48 ` [PATCH bpf-next v5 1/3] " Stanislav Fomichev
2021-10-21 21:48 ` [PATCH bpf-next v5 2/3] bpftool: conditionally append / to the progtype Stanislav Fomichev
@ 2021-10-21 21:48 ` Stanislav Fomichev
2021-10-22 23:57 ` [PATCH bpf-next v5 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Andrii Nakryiko
3 siblings, 0 replies; 6+ messages in thread
From: Stanislav Fomichev @ 2021-10-21 21:48 UTC (permalink / raw)
To: netdev, bpf; +Cc: ast, daniel, andrii, Stanislav Fomichev
- update custom loader to search by name, not section name
- update bpftool commands to use proper pin path
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../selftests/bpf/flow_dissector_load.c | 18 +++++++++++-------
.../selftests/bpf/flow_dissector_load.h | 10 ++--------
.../selftests/bpf/test_flow_dissector.sh | 10 +++++-----
3 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/bpf/flow_dissector_load.c b/tools/testing/selftests/bpf/flow_dissector_load.c
index 3fd83b9dc1bf..87fd1aa323a9 100644
--- a/tools/testing/selftests/bpf/flow_dissector_load.c
+++ b/tools/testing/selftests/bpf/flow_dissector_load.c
@@ -17,7 +17,7 @@
const char *cfg_pin_path = "/sys/fs/bpf/flow_dissector";
const char *cfg_map_name = "jmp_table";
bool cfg_attach = true;
-char *cfg_section_name;
+char *cfg_prog_name;
char *cfg_path_name;
static void load_and_attach_program(void)
@@ -25,7 +25,11 @@ static void load_and_attach_program(void)
int prog_fd, ret;
struct bpf_object *obj;
- ret = bpf_flow_load(&obj, cfg_path_name, cfg_section_name,
+ ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
+ if (ret)
+ error(1, 0, "failed to enable libbpf strict mode: %d", ret);
+
+ ret = bpf_flow_load(&obj, cfg_path_name, cfg_prog_name,
cfg_map_name, NULL, &prog_fd, NULL);
if (ret)
error(1, 0, "bpf_flow_load %s", cfg_path_name);
@@ -75,15 +79,15 @@ static void parse_opts(int argc, char **argv)
break;
case 'p':
if (cfg_path_name)
- error(1, 0, "only one prog name can be given");
+ error(1, 0, "only one path can be given");
cfg_path_name = optarg;
break;
case 's':
- if (cfg_section_name)
- error(1, 0, "only one section can be given");
+ if (cfg_prog_name)
+ error(1, 0, "only one prog can be given");
- cfg_section_name = optarg;
+ cfg_prog_name = optarg;
break;
}
}
@@ -94,7 +98,7 @@ static void parse_opts(int argc, char **argv)
if (cfg_attach && !cfg_path_name)
error(1, 0, "must provide a path to the BPF program");
- if (cfg_attach && !cfg_section_name)
+ if (cfg_attach && !cfg_prog_name)
error(1, 0, "must provide a section name");
}
diff --git a/tools/testing/selftests/bpf/flow_dissector_load.h b/tools/testing/selftests/bpf/flow_dissector_load.h
index 7290401ec172..9d0acc2fc6cc 100644
--- a/tools/testing/selftests/bpf/flow_dissector_load.h
+++ b/tools/testing/selftests/bpf/flow_dissector_load.h
@@ -7,7 +7,7 @@
static inline int bpf_flow_load(struct bpf_object **obj,
const char *path,
- const char *section_name,
+ const char *prog_name,
const char *map_name,
const char *keys_map_name,
int *prog_fd,
@@ -23,13 +23,7 @@ static inline int bpf_flow_load(struct bpf_object **obj,
if (ret)
return ret;
- main_prog = NULL;
- bpf_object__for_each_program(prog, *obj) {
- if (strcmp(section_name, bpf_program__section_name(prog)) == 0) {
- main_prog = prog;
- break;
- }
- }
+ main_prog = bpf_object__find_program_by_name(*obj, prog_name);
if (!main_prog)
return -1;
diff --git a/tools/testing/selftests/bpf/test_flow_dissector.sh b/tools/testing/selftests/bpf/test_flow_dissector.sh
index 174b72a64a4c..dbd91221727d 100755
--- a/tools/testing/selftests/bpf/test_flow_dissector.sh
+++ b/tools/testing/selftests/bpf/test_flow_dissector.sh
@@ -26,22 +26,22 @@ if [[ -z $(ip netns identify $$) ]]; then
type flow_dissector
if ! unshare --net $bpftool prog attach pinned \
- /sys/fs/bpf/flow/flow_dissector flow_dissector; then
+ /sys/fs/bpf/flow/_dissect flow_dissector; then
echo "Unexpected unsuccessful attach in namespace" >&2
err=1
fi
- $bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector \
+ $bpftool prog attach pinned /sys/fs/bpf/flow/_dissect \
flow_dissector
if unshare --net $bpftool prog attach pinned \
- /sys/fs/bpf/flow/flow_dissector flow_dissector; then
+ /sys/fs/bpf/flow/_dissect flow_dissector; then
echo "Unexpected successful attach in namespace" >&2
err=1
fi
if ! $bpftool prog detach pinned \
- /sys/fs/bpf/flow/flow_dissector flow_dissector; then
+ /sys/fs/bpf/flow/_dissect flow_dissector; then
echo "Failed to detach flow dissector" >&2
err=1
fi
@@ -95,7 +95,7 @@ else
fi
# Attach BPF program
-./flow_dissector_load -p bpf_flow.o -s flow_dissector
+./flow_dissector_load -p bpf_flow.o -s _dissect
# Setup
tc qdisc add dev lo ingress
--
2.33.0.1079.g6e70778dc9-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v5 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME
2021-10-21 21:48 [PATCH bpf-next v5 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
` (2 preceding siblings ...)
2021-10-21 21:48 ` [PATCH bpf-next v5 3/3] selftests/bpf: fix flow dissector tests Stanislav Fomichev
@ 2021-10-22 23:57 ` Andrii Nakryiko
3 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2021-10-22 23:57 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
On Thu, Oct 21, 2021 at 2:48 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Commit 15669e1dcd75 ("selftests/bpf: Normalize all the rest SEC() uses")
> broke flow dissector tests. With the strict section names, bpftool isn't
> able to pin all programs of the objects (all section names are the
> same now). To bring it back to life let's do the following:
>
> - teach libbpf to pin by func name with LIBBPF_STRICT_SEC_NAME
> - enable strict mode in bpftool (breaking cli change)
> - fix custom flow_dissector loader to use strict mode
> - fix flow_dissector tests to use new pin names (func vs sec)
>
> v5:
> - get rid of error when retrying with '/' (Quentin Monnet)
>
> v4:
> - fix comment spelling (Quentin Monnet)
> - retry progtype without / (Quentin Monnet)
>
> v3:
> - clarify program pinning in LIBBPF_STRICT_SEC_NAME,
> for real this time (Andrii Nakryiko)
> - fix possible segfault in __bpf_program__pin_name (Andrii Nakryiko)
>
> v2:
> - add github issue (Andrii Nakryiko)
> - remove sec_name from bpf_program.pin_name comment (Andrii Nakryiko)
> - add cover letter (Andrii Nakryiko)
>
> Stanislav Fomichev (3):
> libbpf: use func name when pinning programs with
> LIBBPF_STRICT_SEC_NAME
> bpftool: conditionally append / to the progtype
> selftests/bpf: fix flow dissector tests
I've applied patches #1 and #3, as they have to happen regardless of
how bpftool incompatibility is going to be handled. Please see
comments from John about bpftool. I think we should try to preserve
bpftool's backwards compatibility, or at the very least give users
some way to fall back to non-strict mode during the transition period.
I trust you, John and Quentin will figure out the best way forward
there.
Also, please move this flow_dissector_load selftest into test_progs,
so that we exercise it regularly.
>
> tools/bpf/bpftool/main.c | 4 +++
> tools/bpf/bpftool/prog.c | 35 ++++++++++---------
> tools/lib/bpf/libbpf.c | 13 +++++--
> tools/lib/bpf/libbpf_legacy.h | 3 ++
> .../selftests/bpf/flow_dissector_load.c | 18 ++++++----
> .../selftests/bpf/flow_dissector_load.h | 10 ++----
> .../selftests/bpf/test_flow_dissector.sh | 10 +++---
> 7 files changed, 55 insertions(+), 38 deletions(-)
>
> --
> 2.33.0.1079.g6e70778dc9-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread