netdev - Re: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Oct 2021 21:27:42 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     John Fastabend <john.fastabend@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev <sdf@...gle.com> wrote:
>
> On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
> <john.fastabend@...il.com> wrote:
> >
> > Stanislav Fomichev wrote:
> > > Otherwise, attaching with bpftool doesn't work with strict section names.
> > >
> > > Also, switch to libbpf strict mode to use the latest conventions
> > > (note, I don't think we have any cli api guarantees?).
> > >
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
> > >  tools/bpf/bpftool/main.c |  4 ++++
> > >  tools/bpf/bpftool/prog.c | 15 +--------------
> > >  2 files changed, 5 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> > > index 02eaaf065f65..8223bac1e401 100644
> > > --- a/tools/bpf/bpftool/main.c
> > > +++ b/tools/bpf/bpftool/main.c
> > > @@ -409,6 +409,10 @@ int main(int argc, char **argv)
> > >       block_mount = false;
> > >       bin_name = argv[0];
> > >
> > > +     ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
> > > +     if (ret)
> > > +             p_err("failed to enable libbpf strict mode: %d", ret);
> > > +
> >
> > Would it better to just warn? Seems like this shouldn't be fatal from
> > bpftool side?
> >
> > Also this is a potentially breaking change correct? Programs that _did_
> > work in the unstrict might suddently fail in the strict mode? If this
> > is the case whats the versioning plan? We don't want to leak these
> > type of changes across multiple versions, idealy we have a hard
> > break and bump the version.
> >
> > I didn't catch a cover letter on the series. A small
> > note about versioning and upgrading bpftool would be helpful.
>
> Yeah, it is a breaking change, every program that has non-strict
> section names will be rejected.
>
> I mentioned that in the bpftool's commit description:
> Also, switch to libbpf strict mode to use the latest conventions
> (note, I don't think we have any cli api guarantees?).
>
> So I'm actually not sure what's the best way to handle this migration
> and whether we really provide any cli guarantees to the users. I was
> always assuming that bpftool is mostly for debugging/introspection,
> but not sure.
>
> As Andrii suggested in another email, I can add a flag to disable this
> strict mode. Any better ideas?

Maybe the other way around for the transition period. Add a --strict
flag to turn on libbpf strict mode? This follows libbpf's opt-in
approach to breaking change. We can also emit warnings when people are
trying to pin programs and mention that they should switch to --strict
as in some future version this will be the default. Would that be
better for users?

>
>
>
>
> > >       hash_init(prog_table.table);
> > >       hash_init(map_table.table);
> > >       hash_init(link_table.table);
> > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > > index 277d51c4c5d9..17505dc1243e 100644
> > > --- a/tools/bpf/bpftool/prog.c
> > > +++ b/tools/bpf/bpftool/prog.c
> > > @@ -1396,8 +1396,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) {
> > > @@ -1407,19 +1405,8 @@ 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 = get_prog_type_by_name(type, &common_prog_type,
> > > +                     err = get_prog_type_by_name(*argv, &common_prog_type,
> > >                                                   &expected_attach_type);
> > > -                     free(type);
> > >                       if (err < 0)
> > >                               goto err_free_reuse_maps;
> >
> > This wont potentially break existing programs correct? It looks like
> > just adding a '/' should be fine.
> >
> > Thanks,
> > John