Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 25 Jul 2018 18:48:35 -0700
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Thomas Richter <tmricht@linux.ibm.com>
Cc:     daniel@iogearbox.net, ast@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, heiko.carstens@de.ibm.com,
        brueckner@linux.vnet.ibm.com, schwidefsky@de.ibm.com,
        wangnan0@huawei.com
Subject: Re: [PATCH v2] perf build: Build error in libbpf with
 EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2"
Message-ID: <20180725184835.5d37bef4@cakuba.netronome.com>
In-Reply-To: <20180725072126.2232-1-tmricht@linux.ibm.com>
References: <20180725072126.2232-1-tmricht@linux.ibm.com>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 25 Jul 2018 09:21:26 +0200, Thomas Richter wrote:
> commit a5b8bd47dcc57 ("bpf tools: Collect eBPF programs from their own se=
ctions")

Hmm.. are you sure it's not 531b014e7a2f ("tools: bpf: make use of
reallocarray") that caused the issue?  That commit made us switch from
XSI-compliant to GNU-specific strerror_r() implementation..

/me checks

Yes it looks like 531b014e7a2f~ builds just fine.

Daniel, did you try to apply v1 to the bpf tree?  Perhaps there is a
confusion about the trees here, if this is caused by my recent change
it's a bpf-next material.  strerror() works, but strerror_r() seems
nicer, so perhaps we could keep it if the patch worked in bpf-next?

> causes a compiler error when building the perf tool in the linux-next tre=
e.
> I compile it using a FEDORA 28 installation, my gcc compiler version:
> gcc (GCC) 8.0.1 20180324 (Red Hat 8.0.1-0.20)
>=20
> The file that causes the error is tools/lib/bpf/libbpf.c
>=20
> Here is the error message:
>=20
> [root@p23lp27] # make V=3D1 EXTRA_CFLAGS=3D"-Wp,-D_FORTIFY_SOURCE=3D2 -O2"
> [...]
> make -f /home6/tmricht/linux-next/tools/build/Makefile.build
> 	dir=3D./util/scripting-engines obj=3Dlibperf
> libbpf.c: In function =E2=80=98bpf_object__elf_collect=E2=80=99:
> libbpf.c:811:15: error: ignoring return value of =E2=80=98strerror_r=E2=
=80=99,
> 		declared with attribute warn_unused_result [-Werror=3Dunused-result]
>      strerror_r(-err, errmsg, sizeof(errmsg));
>                ^
> cc1: all warnings being treated as errors
> mv: cannot stat './.libbpf.o.tmp': No such file or directory
> /home6/tmricht/linux-next/tools/build/Makefile.build:96: recipe for targe=
t 'libbpf.o' failed
>=20
> Since this is the only occurance of strerror_r() replace it
> by strerror(). The additional functionality of strerror_r() to
> copy the error message into the supplied buffer is not needed.
> This is also consistant with all the other pr_warning() statements
> in this file which all use strerror().
>=20
> Also fixes a possible initialization issue.
>=20
> Cc: Wang Nan <wangnan0@huawei.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>
>  tools/lib/bpf/libbpf.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>=20
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 955f8eafbf41..f9eb68ff2f4f 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -806,11 +806,8 @@ static int bpf_object__elf_collect(struct bpf_object=
 *obj)
>  			err =3D bpf_object__add_program(obj, data->d_buf,
>  						      data->d_size, name, idx);
>  			if (err) {
> -				char errmsg[STRERR_BUFSIZE];
> -
> -				strerror_r(-err, errmsg, sizeof(errmsg));
>  				pr_warning("failed to alloc program %s (%s): %s",
> -					   name, obj->path, errmsg);
> +					   name, obj->path, strerror(-err));
>  			}
>  		} else if (sh.sh_type =3D=3D SHT_REL) {
>  			void *reloc =3D obj->efile.reloc;
> @@ -2334,7 +2331,7 @@ bpf_perf_event_read_simple(void *mem, unsigned long=
 size,
>  	__u64 data_tail =3D header->data_tail;
>  	__u64 data_head =3D header->data_head;
>  	void *base, *begin, *end;
> -	int ret;
> +	int ret =3D 0;
> =20
>  	asm volatile("" ::: "memory"); /* in real code it should be smp_rmb() */
>  	if (data_head =3D=3D data_tail)

This looks like a separate issue.  The ret variable should really be
enum bpf_perf_event_ret, so could you please initialize it to one of the
values of this enum?

The uninitilized condition can only happen if (data_head !=3D data_tail)
but at the same time (data_head % size =3D=3D data_tail % size) which
should never really happen...  Perhaps initializing to
LIBBPF_PERF_EVENT_ERROR would make sense?

Or better still adding:

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index f732237610e5..fa5a25945f19 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2289,6 +2289,8 @@ bpf_perf_event_read_simple(void *mem, unsigned long s=
ize,
=20
        begin =3D base + data_tail % size;
        end =3D base + data_head % size;
+       if (being =3D=3D end)
+               return LIBBPF_PERF_EVENT_ERROR;
=20
        while (begin !=3D end) {
                struct perf_event_header *ehdr;