From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH v2] perf build: Build error in libbpf with EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2" Date: Fri, 27 Jul 2018 10:57:57 -0700 Message-ID: <20180727105757.2d851920@cakuba.netronome.com> References: <20180725072126.2232-1-tmricht@linux.ibm.com> <20180725184835.5d37bef4@cakuba.netronome.com> <08b6ed57-40a4-7468-b78d-f2a2c5ab10a8@iogearbox.net> <325e3d58-89fb-1b50-8865-22c6f372671a@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: Daniel Borkmann , 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 To: Thomas-Mich Richter Return-path: Received: from mail-pl0-f66.google.com ([209.85.160.66]:37306 "EHLO mail-pl0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388942AbeG0TVA (ORCPT ); Fri, 27 Jul 2018 15:21:00 -0400 Received: by mail-pl0-f66.google.com with SMTP id 31-v6so2633162plc.4 for ; Fri, 27 Jul 2018 10:58:01 -0700 (PDT) In-Reply-To: <325e3d58-89fb-1b50-8865-22c6f372671a@linux.ibm.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, 27 Jul 2018 09:22:03 +0200, Thomas-Mich Richter wrote: > On 07/27/2018 04:16 AM, Daniel Borkmann wrote: > > On 07/26/2018 03:48 AM, Jakub Kicinski wrote: =20 > >> On Wed, 25 Jul 2018 09:21:26 +0200, Thomas Richter wrote: =20 > >>> commit a5b8bd47dcc57 ("bpf tools: Collect eBPF programs from their ow= n sections") =20 > >> > >> 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? =20 > >=20 > > Yeah indeed, the issue is only in bpf-next. When I compile libbpf from > > bpf tree with the below flags then it's all good>=20 > > Agree that we should rather use strerror_r() given this is a library. = =20 >=20 > Are you sure to stick with strerror_r? I ask because it is the only > occurence of strerror_r in this file. All other error messages use strerr= or > as in: > pr_warning("failed to create map (name: '%s'): %s\n", > map->name, > strerror(errno)); >=20 >=20 > $ fgrep strerror tools/lib/bpf/libbpf.c > strerror(errno)); > issue I try to solve---> strerror_r(-err, errmsg, sizeof(errms= g)); > map->name, strerror(errno), errno); > strerror(errno)); > pr_warning("load bpf program failed: %s\n", strerror(errno)); > pr_warning("failed to statfs %s: %s\n", dir, strerror(err= no)); > pr_warning("failed to pin program: %s\n", strerror(errno)= ); > pr_warning("failed to mkdir %s: %s\n", path, strerror(-er= r)); > pr_warning("failed to pin map: %s\n", strerror(errno)); > $ >=20 > The next issue with strerror_r is to assign the return value to a variabl= e. > Then we end up with variable set but not used: > libbpf.c: In function =E2=80=98bpf_object__elf_collect=E2=80=99: > libbpf.c:809:35: error: variable =E2=80=98cp=E2=80=99 set but not used [-= Werror=3Dunused-but-set-variable] > char errmsg[STRERR_BUFSIZE], *cp; > ^ > cc1: all warnings being treated as errors The GNU-specific strerror_r() returns a pointer to a string contain= ing the error message. This may be either a pointer to a string that = the function stores in buf, or a pointer to some (immutable) static str= ing (in which case buf is unused). If the function stores a string in b= uf, then at most buflen bytes are stored (the string may be truncated = if buflen is too small and errnum is unknown). The string always inclu= des a terminating null byte ('\0'). IOW you gotta use the return value. > Here is the source: > if (err) { > char errmsg[STRERR_BUFSIZE], *cp; >=20 > cp =3D strerror_r(-err, errmsg, sizeof(er= rmsg)); > pr_warning("failed to alloc program %s (%= s): %s", > name, obj->path, errmsg); > } >=20 > To fix this requires something like: > pr_warning("failed to alloc program %s (%s= ): %s", > name, obj->path, cp); This looks correct. > And after pr_warning() is done, the local array errmsg is deleted. >=20 > A lot of overkill in my opinion, unless I miss something. IMO using potentially mutli-thread unsafe functions in a library is not optimal, we should strive to convert the other instances of strerror() rather than move the other way. > >>> causes a compiler error when building the perf tool in the linux-next= tree. > >>> 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) > >>> > >>> The file that causes the error is tools/lib/bpf/libbpf.c > >>> > >>> Here is the error message: =20 > > [...] =20 > >>> @@ -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) =20 > >> > >> 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 t= he > >> values of this enum? > >> > >> The uninitilized condition can only happen if (data_head !=3D data_tai= l) > >> 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 l= ong size, > >> =20 > >> begin =3D base + data_tail % size; > >> end =3D base + data_head % size; > >> + if (being =3D=3D end) > >> + return LIBBPF_PERF_EVENT_ERROR; =20 > >=20 > > Sounds good to me. > > =20 >=20 > If you want I can send you a separate patch for this. As far as I'm concerned - yes, please!