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]

Message-ID: <CAKH8qBuR4bYn1POgu0TF428vApknvMNPAng5qMuiKXCpcg8CQQ@mail.gmail.com>
Date:   Mon, 25 Oct 2021 08:58:59 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org
Subject: Re: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

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?




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