netdev - Re: [PATCH bpf-next v4 2/3] bpftool: conditionally 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]

Date:   Thu, 21 Oct 2021 22:10:46 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Stanislav Fomichev <sdf@...gle.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 v4 2/3] bpftool: conditionally append / to the progtype

On Thu, 21 Oct 2021 at 21:40, Stanislav Fomichev <sdf@...gle.com> wrote:
>
> On Thu, Oct 21, 2021 at 12:55 PM Quentin Monnet <quentin@...valent.com> wrote:
> >
> > 2021-10-21 09:56 UTC-0700 ~ Stanislav Fomichev <sdf@...gle.com>
> > > 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?).
> > >
> > > Cc: Quentin Monnet <quentin@...valent.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
> > >  tools/bpf/bpftool/main.c | 4 ++++
> > >  tools/bpf/bpftool/prog.c | 9 +++++++--
> > >  2 files changed, 11 insertions(+), 2 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);
> > > +
> > >       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..b04990588ccf 100644
> > > --- a/tools/bpf/bpftool/prog.c
> > > +++ b/tools/bpf/bpftool/prog.c
> > > @@ -1420,8 +1420,13 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> > >                       err = get_prog_type_by_name(type, &common_prog_type,
> > >                                                   &expected_attach_type);
> > >                       free(type);
> > > -                     if (err < 0)
> > > -                             goto err_free_reuse_maps;
> >
> > Thanks a lot for the change! Can you please test it for e.g. an XDP
> > program? You should see that "bpftool prog load prog.o <path> type xdp"
> > prints a debug message from libbpf about the first attempt (above)
> > failing, before the second attempt (below) succeeds.
> >
> > We need to get rid of this message. I think it should be easy, because
> > we explicitly "ask" for that message in get_prog_type_by_name(), in the
> > same file, if it fails to load in the first place.
> >
> > Could you please update get_prog_type_by_name() to take an additional
> > switch as an argument, to tell if the debug-info should be retrieved
> > (then first attempt here would skip it, second would keep it)?
> > An alternative could be to move all the '/' and retries handling to that
> > function, and I think it would end up in bpftool keeping support for the
> > legacy object files with the former convention - but that would somewhat
> > defeat the objectives of the strict mode, so maybe not the best option.
>
> How about we call libbpf_prog_type_by_name with the provided argv
> first and then, if it doesn't work, we fallback to appending '\' and
> using get_prog_type_by_name ?

Yes it should work, too. Not sure of the order, maybe best to run
_with_ the '/' first, so that the debug message from libbpf (if
neither attempt succeeds) doesn't have the added slash? But that
doesn't matter much anyway.

>
> > > +                     if (err < 0) {
> >
> > We could run the second attempt only on libbpf returning -ESRCH, maybe?
>
> Not sure it matters here, why not always retry on error?

In case the function failed for some other reason and we knew that
retrying with a '/' wouldn't work any better. But in practice that's
only if the name is NULL, and this wouldn't happen because we would
not reach that point, so right, doesn't matter much.