netdev - Re: [PATCH v2 bpf-next 2/2] tools/bpftool: fix bpftool build with bintutils >= 2.8

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: <20171227023204.eulgkbg7epj7nl76@ast-mbp>
Date:   Tue, 26 Dec 2017 18:32:05 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
Cc:     Roman Gushchin <guro@...com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@...com,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH v2 bpf-next 2/2] tools/bpftool: fix bpftool build with
 bintutils >= 2.8

On Fri, Dec 22, 2017 at 06:50:01PM +0000, Quentin Monnet wrote:
> Hi Roman,
> 
> 2017-12-22 16:11 UTC+0000 ~ Roman Gushchin <guro@...com>
> > Bpftool build is broken with binutils version 2.28 and later.
> 
> Could you check the binutils version? I believe it changed in 2.29
> instead of 2.28. Could you update your commit log and subject
> accordingly, please?
> 
> > The cause is commit 003ca0fd2286 ("Refactor disassembler selection")
> > in the binutils repo, which changed the disassembler() function
> > signature.
> > 
> > Fix this by adding a new "feature" to the tools/build/features
> > infrastructure and make it responsible for decision which
> > disassembler() function signature to use.
> > 
> > Signed-off-by: Roman Gushchin <guro@...com>
> > Cc: Jakub Kicinski <jakub.kicinski@...ronome.com>
> > Cc: Alexei Starovoitov <ast@...nel.org>
> > Cc: Daniel Borkmann <daniel@...earbox.net>
> > ---
> >  tools/bpf/Makefile                                | 29 +++++++++++++++++++++++
> >  tools/bpf/bpf_jit_disasm.c                        |  7 ++++++
> >  tools/bpf/bpftool/Makefile                        | 24 +++++++++++++++++++
> >  tools/bpf/bpftool/jit_disasm.c                    |  7 ++++++
> >  tools/build/feature/Makefile                      |  4 ++++
> >  tools/build/feature/test-disassembler-four-args.c | 15 ++++++++++++
> >  6 files changed, 86 insertions(+)
> >  create mode 100644 tools/build/feature/test-disassembler-four-args.c
> > 
> > diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
> > index 07a6697466ef..c8ec0ae16bf0 100644
> > --- a/tools/bpf/Makefile
> > +++ b/tools/bpf/Makefile
> > @@ -9,6 +9,35 @@ MAKE = make
> >  CFLAGS += -Wall -O2
> >  CFLAGS += -D__EXPORTED_HEADERS__ -I../../include/uapi -I../../include
> >  
> > +ifeq ($(srctree),)
> > +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> > +srctree := $(patsubst %/,%,$(dir $(srctree)))
> > +endif
> > +
> > +FEATURE_USER = .bpf
> > +FEATURE_TESTS = libbfd disassembler-four-args
> > +FEATURE_DISPLAY = libbfd disassembler-four-args
> 
> Thanks for adding libbfd as I requested. However, you do not use it in
> the Makefile to prevent compilation if the feature is not detected (see
> "bpfdep" or "elfdep" in tools/lib/bpf/Makefile. Sorry, I should have
> pointed it in my previous review.
> 
> But actually, I have another issue related to the libbfd feature: since
> commit 280e7c48c3b8 ("perf tools: fix BFD detection on opensuse") it
> requires libiberty so that libbfd is correctly detected, but libiberty
> is not needed on all distros (at least Ubuntu can have libbfd without
> libiberty). Typically, detection fails on my setup, although I do have
> libbfd installed. So forcing libbfd feature here may eventually force
> users to install libraries they do not need to compile bpftool, which is
> not what we want.
> 
> I do not have a clean work around to suggest. Maybe have one
> "libbfd-something" feature that tries to compile without libiberty, then
> another one that tries with it, and compile the tools if at least one of
> them succeeds. But it's probably for another patch series. In the
> meantime, would you please simply remove libbfd detection here and
> accept my apologies for suggesting to add it in the previous review?

I think since libbfd is already used by bpftool it's a good thing
to add feature detection. Even if it's not perfect on some setups.

Roman,
I think you still need to do one more respin to address commit log nit?