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

On Fri, Nov 5, 2021 at 9:41 AM Stanislav Fomichev [off-list ref] wrote:

On Fri, Nov 5, 2021 at 4:02 AM Quentin Monnet [off-list ref] wrote:

quoted

2021-11-04 09:03 UTC-0700 ~ Stanislav Fomichev [off-list ref]

quoted

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 <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Stanislav Fomichev <redacted>

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

quoted

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.

quoted

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

quoted

quoted

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

quoted

quoted

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