2021-11-04 09:03 UTC-0700 ~ Stanislav Fomichev <sdf@xxxxxxxxxx> > 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@xxxxxxxxxxxxx> > Cc: John Fastabend <john.fastabend@xxxxxxxxx> > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx> 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
