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

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

[Stanislav Fomichev]

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 <sdf@google.com>
---
 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

[PATCH bpf-next 2/3] bpftool: don't append / to the progtype

[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

[PATCH bpf-next 3/3] selftests/bpf: fix flow dissector tests

[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

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

[Andrii Nakryiko]

On Mon, Oct 11, 2021 at 5:56 PM Stanislav Fomichev <sdf@google.com> 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 <sdf@google.com>
> ---

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

>  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

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.

>          * 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);

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
>

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

[Andrii Nakryiko]

On Mon, Oct 11, 2021 at 5:56 PM Stanislav Fomichev <sdf@google.com> 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 <sdf@google.com>
> ---

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

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

RE: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

[John Fastabend]

Stanislav Fomichev 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);
> +	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.


>  	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

RE: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

[John Fastabend]

John Fastabend wrote:
> Stanislav Fomichev 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);
> > +	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.
> 
> 
> >  	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.

Re: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

[Stanislav Fomichev]

On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Stanislav Fomichev 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);
> > +     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?




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

Re: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

[Andrii Nakryiko]

On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
> <john.fastabend@gmail.com> wrote:
> >
> > Stanislav Fomichev 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);
> > > +     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?

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

Re: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

[Stanislav Fomichev]

On Mon, Oct 25, 2021 at 9:27 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
> > <john.fastabend@gmail.com> wrote:
> > >
> > > Stanislav Fomichev 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);
> > > > +     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?

Re: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

[Andrii Nakryiko]

On Tue, Oct 26, 2021 at 8:46 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Mon, Oct 25, 2021 at 9:27 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
> > > <john.fastabend@gmail.com> wrote:
> > > >
> > > > Stanislav Fomichev 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);
> > > > > +     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).