| 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
| ||
|
Date: Fri, 27 Jul 2018 10:57:57 -0700
From: Jakub Kicinski <jakub.kicinski@...ronome.com>
To: Thomas-Mich Richter <tmricht@...ux.ibm.com>
Cc: Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
heiko.carstens@...ibm.com, brueckner@...ux.vnet.ibm.com,
schwidefsky@...ibm.com, wangnan0@...wei.com
Subject: Re: [PATCH v2] perf build: Build error in libbpf with
EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2"
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:
> >> On Wed, 25 Jul 2018 09:21:26 +0200, Thomas Richter wrote:
> >>> commit a5b8bd47dcc57 ("bpf tools: Collect eBPF programs from their own sections")
> >>
> >> 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?
> >
> > 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>
> > Agree that we should rather use strerror_r() given this is a library.
>
> 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 strerror
> as in:
> pr_warning("failed to create map (name: '%s'): %s\n",
> map->name,
> strerror(errno));
>
>
> $ fgrep strerror tools/lib/bpf/libbpf.c
> strerror(errno));
> issue I try to solve---> strerror_r(-err, errmsg, sizeof(errmsg));
> 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(errno));
> pr_warning("failed to pin program: %s\n", strerror(errno));
> pr_warning("failed to mkdir %s: %s\n", path, strerror(-err));
> pr_warning("failed to pin map: %s\n", strerror(errno));
> $
>
> The next issue with strerror_r is to assign the return value to a variable.
> Then we end up with variable set but not used:
> libbpf.c: In function ‘bpf_object__elf_collect’:
> libbpf.c:809:35: error: variable ‘cp’ set but not used [-Werror=unused-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 containing
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 string
(in which case buf is unused). If the function stores a string in buf,
then at most buflen bytes are stored (the string may be truncated if
buflen is too small and errnum is unknown). The string always includes
a terminating null byte ('\0').
IOW you gotta use the return value.
> Here is the source:
> if (err) {
> char errmsg[STRERR_BUFSIZE], *cp;
>
> cp = strerror_r(-err, errmsg, sizeof(errmsg));
> pr_warning("failed to alloc program %s (%s): %s",
> name, obj->path, errmsg);
> }
>
> 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.
>
> 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:
> > [...]
> >>> @@ -2334,7 +2331,7 @@ bpf_perf_event_read_simple(void *mem, unsigned long size,
> >>> __u64 data_tail = header->data_tail;
> >>> __u64 data_head = header->data_head;
> >>> void *base, *begin, *end;
> >>> - int ret;
> >>> + int ret = 0;
> >>>
> >>> asm volatile("" ::: "memory"); /* in real code it should be smp_rmb() */
> >>> if (data_head == 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 != data_tail)
> >> but at the same time (data_head % size == 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 size,
> >>
> >> begin = base + data_tail % size;
> >> end = base + data_head % size;
> >> + if (being == end)
> >> + return LIBBPF_PERF_EVENT_ERROR;
> >
> > Sounds good to me.
> >
>
> If you want I can send you a separate patch for this.
As far as I'm concerned - yes, please!
Powered by blists - more mailing lists