[PATCH bpf-next] bpftool: add option to enable libbpf's strict mode

[PATCH bpf-next] bpftool: add option to enable libbpf's strict mode

[Stanislav Fomichev]

Otherwise, attaching with bpftool doesn't work with strict section names.

Also:

- by default, don't append / to the section name; in strict
  mode it's relevant only for a small subset of prog types
- print a deprecation warning when requested to pin all programs

+ bpftool prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe
Warning: pinning by section name is deprecated, use --strict to pin by function name.
See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences

+ bpftool prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp
Warning: pinning by section name is deprecated, use --strict to pin by function name.
See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences

+ bpftool --strict prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe
+ bpftool --strict prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp

Cc: Quentin Monnet <quentin@isovalent.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 .../bpftool/Documentation/common_options.rst  |  6 +++
 tools/bpf/bpftool/main.c                      | 13 +++++-
 tools/bpf/bpftool/main.h                      |  1 +
 tools/bpf/bpftool/prog.c                      | 40 +++++++++++--------
 4 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/common_options.rst b/tools/bpf/bpftool/Documentation/common_options.rst
index 05d06c74dcaa..28710f9005be 100644
--- a/tools/bpf/bpftool/Documentation/common_options.rst
+++ b/tools/bpf/bpftool/Documentation/common_options.rst
@@ -20,3 +20,9 @@
 	  Print all logs available, even debug-level information. This includes
 	  logs from libbpf as well as from the verifier, when attempting to
 	  load programs.
+
+-S, --strict
+	  Use strict (aka v1.0) libbpf mode which has more stringent section
+	  name requirements.
+	  See https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
+	  for details.
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 28237d7cef67..10c72089e599 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -31,6 +31,7 @@ bool block_mount;
 bool verifier_logs;
 bool relaxed_maps;
 bool use_loader;
+bool strict_libbpf;
 struct btf *base_btf;
 struct hashmap *refs_table;
 
@@ -396,6 +397,7 @@ int main(int argc, char **argv)
 		{ "debug",	no_argument,	NULL,	'd' },
 		{ "use-loader",	no_argument,	NULL,	'L' },
 		{ "base-btf",	required_argument, NULL, 'B' },
+		{ "strict",	no_argument,	NULL,	'S' },
 		{ 0 }
 	};
 	int opt, ret;
@@ -408,7 +410,7 @@ int main(int argc, char **argv)
 	bin_name = argv[0];
 
 	opterr = 0;
-	while ((opt = getopt_long(argc, argv, "VhpjfLmndB:",
+	while ((opt = getopt_long(argc, argv, "VhpjfLmndB:S",
 				  options, NULL)) >= 0) {
 		switch (opt) {
 		case 'V':
@@ -454,6 +456,9 @@ int main(int argc, char **argv)
 		case 'L':
 			use_loader = true;
 			break;
+		case 'S':
+			strict_libbpf = true;
+			break;
 		default:
 			p_err("unrecognized option '%s'", argv[optind - 1]);
 			if (json_output)
@@ -463,6 +468,12 @@ int main(int argc, char **argv)
 		}
 	}
 
+	if (strict_libbpf) {
+		ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
+		if (ret)
+			p_err("failed to enable libbpf strict mode: %d", ret);
+	}
+
 	argc -= optind;
 	argv += optind;
 	if (argc < 0)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 383835c2604d..b67fa8d8532d 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -90,6 +90,7 @@ extern bool block_mount;
 extern bool verifier_logs;
 extern bool relaxed_maps;
 extern bool use_loader;
+extern bool strict_libbpf;
 extern struct btf *base_btf;
 extern struct hashmap *refs_table;
 
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index dea7a49ec26e..47b321d32b82 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1483,8 +1483,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) {
@@ -1494,21 +1492,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")) {
@@ -1700,6 +1703,11 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 			goto err_close_obj;
 		}
 	} else {
+		if (!strict_libbpf) {
+			p_info("Warning: pinning by section name is deprecated, use --strict to pin by function name.\n"
+			       "See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences\n");
+		}
+
 		err = bpf_object__pin_programs(obj, pinfile);
 		if (err) {
 			p_err("failed to pin all programs");
-- 
2.33.1.1089.g2158813163f-goog

Re: [PATCH bpf-next] bpftool: add option to enable libbpf's strict mode

[Quentin Monnet]

2021-11-04 09:03 UTC-0700 ~ Stanislav Fomichev <sdf@google.com>
> Otherwise, attaching with bpftool doesn't work with strict section names.
> 
> Also:
> 
> - by default, don't append / to the section name; in strict
>   mode it's relevant only for a small subset of prog types
> - print a deprecation warning when requested to pin all programs
> 
> + bpftool prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe
> Warning: pinning by section name is deprecated, use --strict to pin by function name.
> See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
> 
> + bpftool prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp
> Warning: pinning by section name is deprecated, use --strict to pin by function name.
> See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
> 
> + bpftool --strict prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe
> + bpftool --strict prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp
> 
> Cc: Quentin Monnet <quentin@isovalent.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Hi and thanks Stanislav! I have some reservations on the current
approach, though.

I see the new option is here to avoid breaking the current behaviour.
However:

- Libbpf has the API break scheduled for v1.0, and at this stage we
won't be able to avoid breakage in bpftool's behaviour. This means that,
eventually, "bpftool prog loadall" will load functions by func name and
not section name, that section names with garbage prefixes
('SEC("xdp_my_prog")') will be rejected, and that maps with extra
attributes in their definitions will be rejected. And save for the
pinning path difference, we won't be able to tell from bpftool when this
happens, because this is all handled by libbpf.

- In that context, I'd rather have the strict behaviour being the
default. We could have an option to restore the legacy behaviour
(disabling strict mode) during the transition, but I'd rather avoid
users starting to use everywhere a "--strict" option which becomes
either mandatory in the future or (more likely) a deprecated no-op when
we switch to libbpf v1.0 and break legacy behaviour anyway.

- If we were to keep the current option, I'm not a fan of the "--strict"
name, because from a user point of view, I don't think it reflects well
the change to pinning by function name, for example. But given that the
option interferes with different aspects of the loading process, I don't
really have a better suggestion :/.

Aside from the discussion on this option, the code looks good. The
optional '/' on program types on the command line works well, thanks for
preserving the behaviour on the CLI. Please find also a few more notes
below.

> ---
>  .../bpftool/Documentation/common_options.rst  |  6 +++
>  tools/bpf/bpftool/main.c                      | 13 +++++-
>  tools/bpf/bpftool/main.h                      |  1 +
>  tools/bpf/bpftool/prog.c                      | 40 +++++++++++--------
>  4 files changed, 43 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/common_options.rst b/tools/bpf/bpftool/Documentation/common_options.rst
> index 05d06c74dcaa..28710f9005be 100644
> --- a/tools/bpf/bpftool/Documentation/common_options.rst
> +++ b/tools/bpf/bpftool/Documentation/common_options.rst
> @@ -20,3 +20,9 @@
>  	  Print all logs available, even debug-level information. This includes
>  	  logs from libbpf as well as from the verifier, when attempting to
>  	  load programs.
> +
> +-S, --strict

The option does not affect all commands, and could maybe be moved to the
pages of the relevant commands. I think that "prog" and "map" are affected?

> +	  Use strict (aka v1.0) libbpf mode which has more stringent section
> +	  name requirements.
> +	  See https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences

There is more than just the pinning difference. The stricter section
name handling and the changes for map declarations (drop of non-BTF and
of unknown attributes) should also affect the way bpftool can load
objects. Even the changes in the ELF section processing might affect the
resulting objects.

> +	  for details.
Note that if we add a command line option, we'd also need to add it to
the interactive help message and bash completion:

https://elixir.bootlin.com/linux/v5.15/source/tools/bpf/bpftool/main.h#L57
https://elixir.bootlin.com/linux/v5.15/source/tools/bpf/bpftool/bash-completion/bpftool#L263

Thanks,
Quentin

Re: [PATCH bpf-next] bpftool: add option to enable libbpf's strict mode

[Stanislav Fomichev]

On Fri, Nov 5, 2021 at 4:02 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2021-11-04 09:03 UTC-0700 ~ Stanislav Fomichev <sdf@google.com>
> > Otherwise, attaching with bpftool doesn't work with strict section names.
> >
> > Also:
> >
> > - by default, don't append / to the section name; in strict
> >   mode it's relevant only for a small subset of prog types
> > - print a deprecation warning when requested to pin all programs
> >
> > + bpftool prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe
> > Warning: pinning by section name is deprecated, use --strict to pin by function name.
> > See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
> >
> > + bpftool prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp
> > Warning: pinning by section name is deprecated, use --strict to pin by function name.
> > See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
> >
> > + bpftool --strict prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe
> > + bpftool --strict prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp
> >
> > Cc: Quentin Monnet <quentin@isovalent.com>
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> Hi and thanks Stanislav! I have some reservations on the current
> approach, though.
>
> I see the new option is here to avoid breaking the current behaviour.
> However:
>
> - Libbpf has the API break scheduled for v1.0, and at this stage we
> won't be able to avoid breakage in bpftool's behaviour. This means that,
> eventually, "bpftool prog loadall" will load functions by func name and
> not section name, that section names with garbage prefixes
> ('SEC("xdp_my_prog")') will be rejected, and that maps with extra
> attributes in their definitions will be rejected. And save for the
> pinning path difference, we won't be able to tell from bpftool when this
> happens, because this is all handled by libbpf.
>
> - In that context, I'd rather have the strict behaviour being the
> default. We could have an option to restore the legacy behaviour
> (disabling strict mode) during the transition, but I'd rather avoid
> users starting to use everywhere a "--strict" option which becomes
> either mandatory in the future or (more likely) a deprecated no-op when
> we switch to libbpf v1.0 and break legacy behaviour anyway.
>
> - If we were to keep the current option, I'm not a fan of the "--strict"
> name, because from a user point of view, I don't think it reflects well
> the change to pinning by function name, for example. But given that the
> option interferes with different aspects of the loading process, I don't
> really have a better suggestion :/.

Yeah, not sure what's the best way here. Strict by default will break
users, but at least we can expect some action. Non-strict by default
will most likely not cause anybody to add --strict or react to that
warning.

I agree with your point though regarding --strict being default at
some point and polluting every bpftool call with it doesn't look good,
so I'll try to switch to strict by default in v2.

RE naming: we can use --legacy-libbpf instead, but it also doesn't
really tell what the real changes are.

> Aside from the discussion on this option, the code looks good. The
> optional '/' on program types on the command line works well, thanks for
> preserving the behaviour on the CLI. Please find also a few more notes
> below.
>
> > ---
> >  .../bpftool/Documentation/common_options.rst  |  6 +++
> >  tools/bpf/bpftool/main.c                      | 13 +++++-
> >  tools/bpf/bpftool/main.h                      |  1 +
> >  tools/bpf/bpftool/prog.c                      | 40 +++++++++++--------
> >  4 files changed, 43 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/common_options.rst b/tools/bpf/bpftool/Documentation/common_options.rst
> > index 05d06c74dcaa..28710f9005be 100644
> > --- a/tools/bpf/bpftool/Documentation/common_options.rst
> > +++ b/tools/bpf/bpftool/Documentation/common_options.rst
> > @@ -20,3 +20,9 @@
> >         Print all logs available, even debug-level information. This includes
> >         logs from libbpf as well as from the verifier, when attempting to
> >         load programs.
> > +
> > +-S, --strict
>
> The option does not affect all commands, and could maybe be moved to the
> pages of the relevant commands. I think that "prog" and "map" are affected?

True, but probably still a good idea to exercise that strict mode
everywhere in the bpftool itself? To make sure other changes don't
break it in a significant way.

> > +       Use strict (aka v1.0) libbpf mode which has more stringent section
> > +       name requirements.
> > +       See https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
>
> There is more than just the pinning difference. The stricter section
> name handling and the changes for map declarations (drop of non-BTF and
> of unknown attributes) should also affect the way bpftool can load
> objects. Even the changes in the ELF section processing might affect the
> resulting objects.

Ack. Will add a better description here to point to the overall list
of libbpf-1.0 changes.

> > +       for details.
> Note that if we add a command line option, we'd also need to add it to
> the interactive help message and bash completion:
>
> https://elixir.bootlin.com/linux/v5.15/source/tools/bpf/bpftool/main.h#L57
> https://elixir.bootlin.com/linux/v5.15/source/tools/bpf/bpftool/bash-completion/bpftool#L263

Thanks, will do!

Re: [PATCH bpf-next] bpftool: add option to enable libbpf's strict mode

[Andrii Nakryiko]

On Fri, Nov 5, 2021 at 9:41 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Fri, Nov 5, 2021 at 4:02 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >
> > 2021-11-04 09:03 UTC-0700 ~ Stanislav Fomichev <sdf@google.com>
> > > Otherwise, attaching with bpftool doesn't work with strict section names.
> > >
> > > Also:
> > >
> > > - by default, don't append / to the section name; in strict
> > >   mode it's relevant only for a small subset of prog types
> > > - print a deprecation warning when requested to pin all programs
> > >
> > > + bpftool prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe
> > > Warning: pinning by section name is deprecated, use --strict to pin by function name.
> > > See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
> > >
> > > + bpftool prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp
> > > Warning: pinning by section name is deprecated, use --strict to pin by function name.
> > > See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
> > >
> > > + bpftool --strict prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe
> > > + bpftool --strict prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp
> > >
> > > Cc: Quentin Monnet <quentin@isovalent.com>
> > > Cc: John Fastabend <john.fastabend@gmail.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > Hi and thanks Stanislav! I have some reservations on the current
> > approach, though.
> >
> > I see the new option is here to avoid breaking the current behaviour.
> > However:
> >
> > - Libbpf has the API break scheduled for v1.0, and at this stage we
> > won't be able to avoid breakage in bpftool's behaviour. This means that,
> > eventually, "bpftool prog loadall" will load functions by func name and
> > not section name, that section names with garbage prefixes
> > ('SEC("xdp_my_prog")') will be rejected, and that maps with extra
> > attributes in their definitions will be rejected. And save for the
> > pinning path difference, we won't be able to tell from bpftool when this
> > happens, because this is all handled by libbpf.
> >
> > - In that context, I'd rather have the strict behaviour being the
> > default. We could have an option to restore the legacy behaviour
> > (disabling strict mode) during the transition, but I'd rather avoid
> > users starting to use everywhere a "--strict" option which becomes
> > either mandatory in the future or (more likely) a deprecated no-op when
> > we switch to libbpf v1.0 and break legacy behaviour anyway.
> >
> > - If we were to keep the current option, I'm not a fan of the "--strict"
> > name, because from a user point of view, I don't think it reflects well
> > the change to pinning by function name, for example. But given that the
> > option interferes with different aspects of the loading process, I don't
> > really have a better suggestion :/.
>
> Yeah, not sure what's the best way here. Strict by default will break
> users, but at least we can expect some action. Non-strict by default
> will most likely not cause anybody to add --strict or react to that
> warning.
>
> I agree with your point though regarding --strict being default at
> some point and polluting every bpftool call with it doesn't look good,
> so I'll try to switch to strict by default in v2.
>
> RE naming: we can use --legacy-libbpf instead, but it also doesn't
> really tell what the real changes are.

maybe --relaxed or just --legacy then?

We can also have a warning or information message pointing to [0] for
details of what is changing? And feel free to contribute with whatever
important things that should be mentioned there as well.

  [0] https://github.com/libbpf/libbpf/wiki/Libbpf-1.0-migration-guide

>
> > Aside from the discussion on this option, the code looks good. The
> > optional '/' on program types on the command line works well, thanks for
> > preserving the behaviour on the CLI. Please find also a few more notes
> > below.
> >
> > > ---
> > >  .../bpftool/Documentation/common_options.rst  |  6 +++
> > >  tools/bpf/bpftool/main.c                      | 13 +++++-
> > >  tools/bpf/bpftool/main.h                      |  1 +
> > >  tools/bpf/bpftool/prog.c                      | 40 +++++++++++--------
> > >  4 files changed, 43 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/tools/bpf/bpftool/Documentation/common_options.rst b/tools/bpf/bpftool/Documentation/common_options.rst
> > > index 05d06c74dcaa..28710f9005be 100644
> > > --- a/tools/bpf/bpftool/Documentation/common_options.rst
> > > +++ b/tools/bpf/bpftool/Documentation/common_options.rst
> > > @@ -20,3 +20,9 @@
> > >         Print all logs available, even debug-level information. This includes
> > >         logs from libbpf as well as from the verifier, when attempting to
> > >         load programs.
> > > +
> > > +-S, --strict
> >
> > The option does not affect all commands, and could maybe be moved to the
> > pages of the relevant commands. I think that "prog" and "map" are affected?
>
> True, but probably still a good idea to exercise that strict mode
> everywhere in the bpftool itself? To make sure other changes don't
> break it in a significant way.
>
> > > +       Use strict (aka v1.0) libbpf mode which has more stringent section
> > > +       name requirements.
> > > +       See https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences
> >
> > There is more than just the pinning difference. The stricter section
> > name handling and the changes for map declarations (drop of non-BTF and
> > of unknown attributes) should also affect the way bpftool can load
> > objects. Even the changes in the ELF section processing might affect the
> > resulting objects.
>
> Ack. Will add a better description here to point to the overall list
> of libbpf-1.0 changes.
>
> > > +       for details.
> > Note that if we add a command line option, we'd also need to add it to
> > the interactive help message and bash completion:
> >
> > https://elixir.bootlin.com/linux/v5.15/source/tools/bpf/bpftool/main.h#L57
> > https://elixir.bootlin.com/linux/v5.15/source/tools/bpf/bpftool/bash-completion/bpftool#L263
>
> Thanks, will do!