netdev - Re: [PATCH bpf-next v2 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] [day] [month] [year] [list]

Message-ID: <CAKH8qBsDGPMnw=302poLcv1eoY+mDVLDttUc3HPQXJoVddbC6g@mail.gmail.com>
Date:   Wed, 20 Oct 2021 15:46:54 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     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 v2 2/3] bpftool: don't append / to the progtype

On Wed, Oct 20, 2021 at 11:12 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Tue, Oct 12, 2021 at 9:15 AM Stanislav Fomichev <sdf@...gle.com> 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);
>
> Quentin, any concerns about turning strict mode for bpftool? Either
> way we should audit bpftool code to ensure that at least error
> handling is done properly (see my comments on Dave's patch set about
> == -1 checks).
>
> > +       if (ret)
> > +               p_err("failed to enable libbpf strict mode: %d", ret);
> > +
> >         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);
>
> Question not specifically to Stanislav, but anyone who's using bpftool
> to load programs. Do we need to support program type overrides? Libbpf
> has been inferring the program type for a long time now, are there
> realistic use cases where this override logic is necessary? I see
> there is bpf_object__for_each_program() loop down the code, it
> essentially repeats what libbpf is already doing on
> bpf_object__open(), why keep the duplicated logic?
>
> libbpf_prog_type_by_name() is also a bad idea (IMO) and I'd like to
> get rid of that in libbpf 1.0, so if we can stop using that from
> bpftool, it would be great.
>
> Thoughts?

IMO it's all legacy at this point. If we can remove / simplify by
calling higher level abstraction from libbpf - there is no reason not
to do it.