netdev - Re: [PATCH v3] tools: bpftool: fix reading from /proc/config.gz

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: <20190809214831.GE2820@mini-arch>
Date:   Fri, 9 Aug 2019 14:48:31 -0700
From:   Stanislav Fomichev <sdf@...ichev.me>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Peter Wu <peter@...ensteyn.nl>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        Stanislav Fomichev <sdf@...gle.com>,
        Quentin Monnet <quentin.monnet@...ronome.com>
Subject: Re: [PATCH v3] tools: bpftool: fix reading from /proc/config.gz

On 08/09, Jakub Kicinski wrote:
> On Fri, 9 Aug 2019 08:32:10 -0700, Stanislav Fomichev wrote:
> > On 08/09, Peter Wu wrote:
> > > /proc/config has never existed as far as I can see, but /proc/config.gz
> > > is present on Arch Linux. Add support for decompressing config.gz using
> > > zlib which is a mandatory dependency of libelf. Replace existing stdio
> > > functions with gzFile operations since the latter transparently handles
> > > uncompressed and gzip-compressed files.
> > > 
> > > Cc: Quentin Monnet <quentin.monnet@...ronome.com>
> > > Signed-off-by: Peter Wu <peter@...ensteyn.nl>
> 
> Thanks for the patch, looks good to me now!
> 
> > >  tools/bpf/bpftool/Makefile  |   2 +-
> > >  tools/bpf/bpftool/feature.c | 105 ++++++++++++++++++------------------
> > >  2 files changed, 54 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> > > index a7afea4dec47..078bd0dcfba5 100644
> > > --- a/tools/bpf/bpftool/Makefile
> > > +++ b/tools/bpf/bpftool/Makefile
> > > @@ -52,7 +52,7 @@ ifneq ($(EXTRA_LDFLAGS),)
> > >  LDFLAGS += $(EXTRA_LDFLAGS)
> > >  endif
> > >  
> > > -LIBS = -lelf $(LIBBPF)
> > > +LIBS = -lelf -lz $(LIBBPF)  
> > You're saying in the commit description that bpftool already links
> > against -lz (via -lelf), but then explicitly add -lz here, why?
> 
> It probably won't hurt to enable the zlib test:
> 
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 078bd0dcfba5..8176632e519c 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -58,8 +58,8 @@ INSTALL ?= install
>  RM ?= rm -f
>  
>  FEATURE_USER = .bpftool
> -FEATURE_TESTS = libbfd disassembler-four-args reallocarray
> -FEATURE_DISPLAY = libbfd disassembler-four-args
> +FEATURE_TESTS = libbfd disassembler-four-args reallocarray zlib
> +FEATURE_DISPLAY = libbfd disassembler-four-args zlib
>  
>  check_feat := 1
>  NON_CHECK_FEAT_TARGETS := clean uninstall doc doc-clean doc-install doc-uninstall
> 
> And then we can test for it the way libbpf tests for elf:
> 
> all: zdep $(OUTPUT)bpftool
> 
> PHONY += zdep
> 
> zdep:
> 	@if [ "$(feature-zlib)" != "1" ]; then echo "No zlib found"; exit 1 ; fi
> 
> Or maybe just $(error ...), Stan what's your preference here? 
> We don't have a precedent for hard tests of features in bpftool.
I'm just being nit picky :-)
Because changelog says we already depend on -lz, but then in the patch
we explicitly add it.

I think you were right in pointing out that we already implicitly depend
on -lz via -lelf and/or -lbfd. And it works for non-static builds.
We don't need an explicit -lz unless somebody puts '-static' in
EXTRA_CFLAGS. So maybe we should just submit the patch as is because
it fixes make EXTRA_CFLAGS=-static.

RE $(error): we don't do it for -lelf, right? So probably not worth
the hassle for zlib.