bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME
@ 2021-10-12 16:15 Stanislav Fomichev
  2021-10-12 16:15 ` [PATCH bpf-next v2 1/3] " Stanislav Fomichev
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2021-10-12 16:15 UTC (permalink / raw)
  To: netdev, bpf; +Cc: ast, daniel, andrii, Stanislav Fomichev

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)

v2:
- add github issue (Andrii Nakryiko)
- remove sec_name from bpf_program.pin_name comment (Andrii Nakryiko)
- clarify program pinning in LIBBPF_STRICT_SEC_NAME (Andrii Nakryiko)
- add cover letter (Andrii Nakryiko)

Stanislav Fomichev (3):
  libbpf: use func name when pinning programs with
    LIBBPF_STRICT_SEC_NAME
  bpftool: don't append / to the progtype
  selftests/bpf: fix flow dissector tests

 tools/bpf/bpftool/main.c                       |  4 ++++
 tools/bpf/bpftool/prog.c                       | 15 +--------------
 tools/lib/bpf/libbpf.c                         | 10 ++++++++--
 .../selftests/bpf/flow_dissector_load.c        | 18 +++++++++++-------
 .../selftests/bpf/flow_dissector_load.h        | 10 ++--------
 .../selftests/bpf/test_flow_dissector.sh       | 10 +++++-----
 6 files changed, 31 insertions(+), 36 deletions(-)

-- 
2.33.0.882.g93a45727a2-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH bpf-next v2 1/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME
  2021-10-12 16:15 [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
@ 2021-10-12 16:15 ` Stanislav Fomichev
  2021-10-20 18:14   ` Andrii Nakryiko
  2021-10-12 16:15 ` [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype Stanislav Fomichev
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Stanislav Fomichev @ 2021-10-12 16:15 UTC (permalink / raw)
  To: netdev, bpf; +Cc: ast, daniel, andrii, Stanislav Fomichev

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
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/lib/bpf/libbpf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ae0889bebe32..d1646b32188f 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,13 @@ 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);
+
+	p = name;
+
 	while ((p = strchr(p, '/')))
 		*p = '_';
 
-- 
2.33.0.882.g93a45727a2-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype
  2021-10-12 16:15 [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
  2021-10-12 16:15 ` [PATCH bpf-next v2 1/3] " Stanislav Fomichev
@ 2021-10-12 16:15 ` Stanislav Fomichev
  2021-10-20 18:12   ` Andrii Nakryiko
  2021-10-12 16:15 ` [PATCH bpf-next v2 3/3] selftests/bpf: fix flow dissector tests Stanislav Fomichev
  2021-10-20 18:14 ` [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Andrii Nakryiko
  3 siblings, 1 reply; 9+ messages in thread
From: Stanislav Fomichev @ 2021-10-12 16:15 UTC (permalink / raw)
  To: netdev, bpf; +Cc: ast, daniel, andrii, Stanislav Fomichev

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?).

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/bpf/bpftool/main.c |  4 ++++
 tools/bpf/bpftool/prog.c | 15 +--------------
 2 files changed, 5 insertions(+), 14 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..17505dc1243e 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,19 +1405,8 @@ 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 = get_prog_type_by_name(type, &common_prog_type,
+			err = get_prog_type_by_name(*argv, &common_prog_type,
 						    &expected_attach_type);
-			free(type);
 			if (err < 0)
 				goto err_free_reuse_maps;
 
-- 
2.33.0.882.g93a45727a2-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH bpf-next v2 3/3] selftests/bpf: fix flow dissector tests
  2021-10-12 16:15 [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
  2021-10-12 16:15 ` [PATCH bpf-next v2 1/3] " Stanislav Fomichev
  2021-10-12 16:15 ` [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype Stanislav Fomichev
@ 2021-10-12 16:15 ` Stanislav Fomichev
  2021-10-20 18:14 ` [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Andrii Nakryiko
  3 siblings, 0 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2021-10-12 16:15 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.882.g93a45727a2-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype
  2021-10-12 16:15 ` [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype Stanislav Fomichev
@ 2021-10-20 18:12   ` Andrii Nakryiko
  2021-10-20 22:46     ` Stanislav Fomichev
  0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 18:12 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Tue, Oct 12, 2021 at 9:15 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> 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?).
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  tools/bpf/bpftool/main.c |  4 ++++
>  tools/bpf/bpftool/prog.c | 15 +--------------
>  2 files changed, 5 insertions(+), 14 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);

Quentin, any concerns about turning strict mode for bpftool? Either
way we should audit bpftool code to ensure that at least error
handling is done properly (see my comments on Dave's patch set about
== -1 checks).

> +       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..17505dc1243e 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,19 +1405,8 @@ 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 = get_prog_type_by_name(type, &common_prog_type,
> +                       err = get_prog_type_by_name(*argv, &common_prog_type,
>                                                     &expected_attach_type);

Question not specifically to Stanislav, but anyone who's using bpftool
to load programs. Do we need to support program type overrides? Libbpf
has been inferring the program type for a long time now, are there
realistic use cases where this override logic is necessary? I see
there is bpf_object__for_each_program() loop down the code, it
essentially repeats what libbpf is already doing on
bpf_object__open(), why keep the duplicated logic?

libbpf_prog_type_by_name() is also a bad idea (IMO) and I'd like to
get rid of that in libbpf 1.0, so if we can stop using that from
bpftool, it would be great.

Thoughts?

> -                       free(type);
>                         if (err < 0)
>                                 goto err_free_reuse_maps;
>
> --
> 2.33.0.882.g93a45727a2-goog
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME
  2021-10-12 16:15 [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
                   ` (2 preceding siblings ...)
  2021-10-12 16:15 ` [PATCH bpf-next v2 3/3] selftests/bpf: fix flow dissector tests Stanislav Fomichev
@ 2021-10-20 18:14 ` Andrii Nakryiko
  2021-10-20 18:22   ` Stanislav Fomichev
  3 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 18:14 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Tue, Oct 12, 2021 at 9:15 AM 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)
>
> v2:
> - add github issue (Andrii Nakryiko)
> - remove sec_name from bpf_program.pin_name comment (Andrii Nakryiko)
> - clarify program pinning in LIBBPF_STRICT_SEC_NAME (Andrii Nakryiko)

I could not find this, can you please point me to where this is
clarified/explained in your patches?

> - add cover letter (Andrii Nakryiko)
>
> Stanislav Fomichev (3):
>   libbpf: use func name when pinning programs with
>     LIBBPF_STRICT_SEC_NAME
>   bpftool: don't append / to the progtype
>   selftests/bpf: fix flow dissector tests
>
>  tools/bpf/bpftool/main.c                       |  4 ++++
>  tools/bpf/bpftool/prog.c                       | 15 +--------------
>  tools/lib/bpf/libbpf.c                         | 10 ++++++++--
>  .../selftests/bpf/flow_dissector_load.c        | 18 +++++++++++-------
>  .../selftests/bpf/flow_dissector_load.h        | 10 ++--------
>  .../selftests/bpf/test_flow_dissector.sh       | 10 +++++-----
>  6 files changed, 31 insertions(+), 36 deletions(-)
>
> --
> 2.33.0.882.g93a45727a2-goog
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next v2 1/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME
  2021-10-12 16:15 ` [PATCH bpf-next v2 1/3] " Stanislav Fomichev
@ 2021-10-20 18:14   ` Andrii Nakryiko
  0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 18:14 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Tue, Oct 12, 2021 at 9:15 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> 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
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  tools/lib/bpf/libbpf.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ae0889bebe32..d1646b32188f 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,13 @@ 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);
> +
> +       p = name;
> +
>         while ((p = strchr(p, '/')))

I bet this will SIGSEGV if p is NULL? Can you please add a check and Fixes: tag?

>                 *p = '_';
>
> --
> 2.33.0.882.g93a45727a2-goog
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME
  2021-10-20 18:14 ` [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Andrii Nakryiko
@ 2021-10-20 18:22   ` Stanislav Fomichev
  0 siblings, 0 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2021-10-20 18:22 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Wed, Oct 20, 2021 at 11:14 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 12, 2021 at 9:15 AM 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)
> >
> > v2:
> > - add github issue (Andrii Nakryiko)
> > - remove sec_name from bpf_program.pin_name comment (Andrii Nakryiko)
> > - clarify program pinning in LIBBPF_STRICT_SEC_NAME (Andrii Nakryiko)
>
> I could not find this, can you please point me to where this is
> clarified/explained in your patches?

Sorry, I don't see it either. I remember I've added some comment to
LIBBPF_STRICT_SEC_NAME in tools/lib/bpf/libbpf_legacy.h but I don't
see it in the paches / my local tree. Will add back and resend.

>
> > - add cover letter (Andrii Nakryiko)
> >
> > Stanislav Fomichev (3):
> >   libbpf: use func name when pinning programs with
> >     LIBBPF_STRICT_SEC_NAME
> >   bpftool: don't append / to the progtype
> >   selftests/bpf: fix flow dissector tests
> >
> >  tools/bpf/bpftool/main.c                       |  4 ++++
> >  tools/bpf/bpftool/prog.c                       | 15 +--------------
> >  tools/lib/bpf/libbpf.c                         | 10 ++++++++--
> >  .../selftests/bpf/flow_dissector_load.c        | 18 +++++++++++-------
> >  .../selftests/bpf/flow_dissector_load.h        | 10 ++--------
> >  .../selftests/bpf/test_flow_dissector.sh       | 10 +++++-----
> >  6 files changed, 31 insertions(+), 36 deletions(-)
> >
> > --
> > 2.33.0.882.g93a45727a2-goog
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype
  2021-10-20 18:12   ` Andrii Nakryiko
@ 2021-10-20 22:46     ` Stanislav Fomichev
  0 siblings, 0 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2021-10-20 22:46 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Wed, Oct 20, 2021 at 11:12 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 12, 2021 at 9:15 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > 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?).
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  tools/bpf/bpftool/main.c |  4 ++++
> >  tools/bpf/bpftool/prog.c | 15 +--------------
> >  2 files changed, 5 insertions(+), 14 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);
>
> Quentin, any concerns about turning strict mode for bpftool? Either
> way we should audit bpftool code to ensure that at least error
> handling is done properly (see my comments on Dave's patch set about
> == -1 checks).
>
> > +       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..17505dc1243e 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,19 +1405,8 @@ 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 = get_prog_type_by_name(type, &common_prog_type,
> > +                       err = get_prog_type_by_name(*argv, &common_prog_type,
> >                                                     &expected_attach_type);
>
> Question not specifically to Stanislav, but anyone who's using bpftool
> to load programs. Do we need to support program type overrides? Libbpf
> has been inferring the program type for a long time now, are there
> realistic use cases where this override logic is necessary? I see
> there is bpf_object__for_each_program() loop down the code, it
> essentially repeats what libbpf is already doing on
> bpf_object__open(), why keep the duplicated logic?
>
> libbpf_prog_type_by_name() is also a bad idea (IMO) and I'd like to
> get rid of that in libbpf 1.0, so if we can stop using that from
> bpftool, it would be great.
>
> Thoughts?

IMO it's all legacy at this point. If we can remove / simplify by
calling higher level abstraction from libbpf - there is no reason not
to do it.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-10-20 22:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12 16:15 [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Stanislav Fomichev
2021-10-12 16:15 ` [PATCH bpf-next v2 1/3] " Stanislav Fomichev
2021-10-20 18:14   ` Andrii Nakryiko
2021-10-12 16:15 ` [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype Stanislav Fomichev
2021-10-20 18:12   ` Andrii Nakryiko
2021-10-20 22:46     ` Stanislav Fomichev
2021-10-12 16:15 ` [PATCH bpf-next v2 3/3] selftests/bpf: fix flow dissector tests Stanislav Fomichev
2021-10-20 18:14 ` [PATCH bpf-next v2 0/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME Andrii Nakryiko
2021-10-20 18:22   ` Stanislav Fomichev

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).