Message-ID: <6fbd1539-c343-2d03-d153-11d2684effd6@isovalent.com>
Date:   Fri, 5 Nov 2021 11:02:09 +0000
From:   Quentin Monnet <quentin@...valent.com>
To:     Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH bpf-next] bpftool: add option to enable libbpf's strict
 mode

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

Powered by blists - more mailing lists