Date:   Fri, 5 Nov 2021 09:41:22 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Quentin Monnet <quentin@...valent.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, 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

On Fri, Nov 5, 2021 at 4:02 AM Quentin Monnet <quentin@...valent.com> wrote:
>
> 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 :/.

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!

Powered by blists - more mailing lists