[PATCH bpf-next 1/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME

We can't use section name anymore because it's not unique
and pinning objects with multiple programs with the same
progtype/secname will fail.

Signed-off-by: Stanislav Fomichev <redacted>
---
 tools/lib/bpf/libbpf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ae0889bebe32..0373ca86a54c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -285,7 +285,8 @@ struct bpf_program {
 	size_t sub_insn_off;
 
 	char *name;
-	/* sec_name with / replaced by _; makes recursive pinning
+	/* sec_name (or name when using LIBBPF_STRICT_SEC_NAME)
+	 * with / replaced by _; makes recursive pinning
 	 * in bpf_object__pin_programs easier
 	 */
 	char *pin_name;

@@ -614,7 +615,11 @@ 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 = p = strdup(prog->name);
+	else
+		name = p = strdup(prog->sec_name);
+
 	while ((p = strchr(p, '/')))
 		*p = '_';
 

-- 
2.33.0.882.g93a45727a2-goog

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 <redacted>
---
 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

- update custom loader to search by name, not section name
- update bpftool commands to use proper pin path

Signed-off-by: Stanislav Fomichev <redacted>
---
 .../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

On Mon, Oct 11, 2021 at 5:56 PM Stanislav Fomichev [off-list ref] wrote:

We can't use section name anymore because it's not unique
and pinning objects with multiple programs with the same
progtype/secname will fail.

Signed-off-by: Stanislav Fomichev <redacted>
---

Seems like you've signed up yourself for [0]? ;)

Please use the following syntax so that when this gets eventually
synced to Github, the issue will be auto-closed.

  [0] Closes: https://github.com/libbpf/libbpf/issues/273

let's remove specific mention of sec_name from this comment. Please
add clarification to LIBBPF_STRICT_SEC_NAME comment instead,
mentioning that it also changes the behavior of pinning.

nit: instead of duplicating this double assignment, you can just do `p
= name;` right before the below loop. It will be a bit cleaner.

+
        while ((p = strchr(p, '/')))
                *p = '_';

--
2.33.0.882.g93a45727a2-goog

On Mon, Oct 11, 2021 at 5:56 PM Stanislav Fomichev [off-list ref] wrote:

We can't use section name anymore because it's not unique
and pinning objects with multiple programs with the same
progtype/secname will fail.

Signed-off-by: Stanislav Fomichev <redacted>
---

Also, patch sets with more than one related patch should come with the
cover letter, please send one on the next revision. Thanks!

Stanislav Fomichev wrote:

Would it better to just warn? Seems like this shouldn't be fatal from
bpftool side?

Also this is a potentially breaking change correct? Programs that _did_
work in the unstrict might suddently fail in the strict mode? If this
is the case whats the versioning plan? We don't want to leak these
type of changes across multiple versions, idealy we have a hard
break and bump the version.

I didn't catch a cover letter on the series. A small
note about versioning and upgrading bpftool would be helpful.

This wont potentially break existing programs correct? It looks like
just adding a '/' should be fine.

Thanks,
John

John Fastabend wrote:

Stanislav Fomichev wrote:

quoted

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 <redacted>
---
 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);
+

Would it better to just warn? Seems like this shouldn't be fatal from
bpftool side?

Also this is a potentially breaking change correct? Programs that _did_
work in the unstrict might suddently fail in the strict mode? If this
is the case whats the versioning plan? We don't want to leak these
type of changes across multiple versions, idealy we have a hard
break and bump the version.

I didn't catch a cover letter on the series. A small
note about versioning and upgrading bpftool would be helpful.

quoted

 	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;

This wont potentially break existing programs correct? It looks like
just adding a '/' should be fine.

Thanks,
John

Oops  wrong version of the patch. I'll reply in the more recent one.

On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
[off-list ref] wrote:

Stanislav Fomichev wrote:

quoted

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 <redacted>
---
 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);
+

Would it better to just warn? Seems like this shouldn't be fatal from
bpftool side?

Also this is a potentially breaking change correct? Programs that _did_
work in the unstrict might suddently fail in the strict mode? If this
is the case whats the versioning plan? We don't want to leak these
type of changes across multiple versions, idealy we have a hard
break and bump the version.

I didn't catch a cover letter on the series. A small
note about versioning and upgrading bpftool would be helpful.

Yeah, it is a breaking change, every program that has non-strict
section names will be rejected.

I mentioned that in the bpftool's commit description:
Also, switch to libbpf strict mode to use the latest conventions
(note, I don't think we have any cli api guarantees?).

So I'm actually not sure what's the best way to handle this migration
and whether we really provide any cli guarantees to the users. I was
always assuming that bpftool is mostly for debugging/introspection,
but not sure.

As Andrii suggested in another email, I can add a flag to disable this
strict mode. Any better ideas?

quoted

      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;

This wont potentially break existing programs correct? It looks like
just adding a '/' should be fine.

Thanks,
John

On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev [off-list ref] wrote:

On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
[off-list ref] wrote:

quoted

Stanislav Fomichev wrote:

quoted

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 <redacted>
---
 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);
+

Would it better to just warn? Seems like this shouldn't be fatal from
bpftool side?

Also this is a potentially breaking change correct? Programs that _did_
work in the unstrict might suddently fail in the strict mode? If this
is the case whats the versioning plan? We don't want to leak these
type of changes across multiple versions, idealy we have a hard
break and bump the version.

I didn't catch a cover letter on the series. A small
note about versioning and upgrading bpftool would be helpful.

Yeah, it is a breaking change, every program that has non-strict
section names will be rejected.

I mentioned that in the bpftool's commit description:
Also, switch to libbpf strict mode to use the latest conventions
(note, I don't think we have any cli api guarantees?).

So I'm actually not sure what's the best way to handle this migration
and whether we really provide any cli guarantees to the users. I was
always assuming that bpftool is mostly for debugging/introspection,
but not sure.

As Andrii suggested in another email, I can add a flag to disable this
strict mode. Any better ideas?

Maybe the other way around for the transition period. Add a --strict
flag to turn on libbpf strict mode? This follows libbpf's opt-in
approach to breaking change. We can also emit warnings when people are
trying to pin programs and mention that they should switch to --strict
as in some future version this will be the default. Would that be
better for users?

quoted

quoted

      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;

This wont potentially break existing programs correct? It looks like
just adding a '/' should be fine.

Thanks,
John

On Mon, Oct 25, 2021 at 9:27 PM Andrii Nakryiko
[off-list ref] wrote:

On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev [off-list ref] wrote:

quoted

On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
[off-list ref] wrote:

quoted

Stanislav Fomichev wrote:

quoted

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 <redacted>
---
 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);
+

Would it better to just warn? Seems like this shouldn't be fatal from
bpftool side?

Also this is a potentially breaking change correct? Programs that _did_
work in the unstrict might suddently fail in the strict mode? If this
is the case whats the versioning plan? We don't want to leak these
type of changes across multiple versions, idealy we have a hard
break and bump the version.

I didn't catch a cover letter on the series. A small
note about versioning and upgrading bpftool would be helpful.

Yeah, it is a breaking change, every program that has non-strict
section names will be rejected.

I mentioned that in the bpftool's commit description:
Also, switch to libbpf strict mode to use the latest conventions
(note, I don't think we have any cli api guarantees?).

So I'm actually not sure what's the best way to handle this migration
and whether we really provide any cli guarantees to the users. I was
always assuming that bpftool is mostly for debugging/introspection,
but not sure.

As Andrii suggested in another email, I can add a flag to disable this
strict mode. Any better ideas?

Maybe the other way around for the transition period. Add a --strict
flag to turn on libbpf strict mode? This follows libbpf's opt-in
approach to breaking change. We can also emit warnings when people are
trying to pin programs and mention that they should switch to --strict
as in some future version this will be the default. Would that be
better for users?

Agreed, that sounds better for backwards compatibility. However, I'm
not sure when we set that --strict to 'true' by default. The same
moment libbpf loses non-strict behavior?

On Tue, Oct 26, 2021 at 8:46 AM Stanislav Fomichev [off-list ref] wrote:

On Mon, Oct 25, 2021 at 9:27 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev [off-list ref] wrote:

quoted

On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
[off-list ref] wrote:

quoted

Stanislav Fomichev wrote:

quoted

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 <redacted>
---
 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);
+

Would it better to just warn? Seems like this shouldn't be fatal from
bpftool side?

Also this is a potentially breaking change correct? Programs that _did_
work in the unstrict might suddently fail in the strict mode? If this
is the case whats the versioning plan? We don't want to leak these
type of changes across multiple versions, idealy we have a hard
break and bump the version.

I didn't catch a cover letter on the series. A small
note about versioning and upgrading bpftool would be helpful.

Yeah, it is a breaking change, every program that has non-strict
section names will be rejected.

I mentioned that in the bpftool's commit description:
Also, switch to libbpf strict mode to use the latest conventions
(note, I don't think we have any cli api guarantees?).

So I'm actually not sure what's the best way to handle this migration
and whether we really provide any cli guarantees to the users. I was
always assuming that bpftool is mostly for debugging/introspection,
but not sure.

As Andrii suggested in another email, I can add a flag to disable this
strict mode. Any better ideas?

Maybe the other way around for the transition period. Add a --strict
flag to turn on libbpf strict mode? This follows libbpf's opt-in
approach to breaking change. We can also emit warnings when people are
trying to pin programs and mention that they should switch to --strict
as in some future version this will be the default. Would that be
better for users?

Agreed, that sounds better for backwards compatibility. However, I'm
not sure when we set that --strict to 'true' by default. The same
moment libbpf loses non-strict behavior?

Yep, probably it will have to coincide with libbpf 1.0 release. And
it's not setting it to true, it's more like enforcing it to true (or
just dropping the --strict flag altogether).