Message ID | 20180725072126.2232-1-tmricht@linux.ibm.com |
---|---|
State | Changes Requested, archived |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [v2] perf build: Build error in libbpf with EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2" | expand |
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? > 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: > > [root@p23lp27] # make V=1 EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2" > [...] > make -f /home6/tmricht/linux-next/tools/build/Makefile.build > dir=./util/scripting-engines obj=libperf > libbpf.c: In function ‘bpf_object__elf_collect’: > libbpf.c:811:15: error: ignoring return value of ‘strerror_r’, > declared with attribute warn_unused_result [-Werror=unused-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 target 'libbpf.o' failed > > 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(). > > Also fixes a possible initialization issue. > > 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(-) > > 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 = 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 == SHT_REL) { > void *reloc = obj->efile.reloc; > @@ -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; while (begin != end) { struct perf_event_header *ehdr;
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. >> 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. > while (begin != end) { > struct perf_event_header *ehdr; >
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 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); And after pr_warning() is done, the local array errmsg is deleted. A lot of overkill in my opinion, unless I miss something. >>> 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.
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!
On 07/27/2018 07:57 PM, Jakub Kicinski wrote: > 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. Yeah, fully agree. We should convert the other ones as well over to use strerror_r(). Thanks, Daniel
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 = 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 == SHT_REL) { void *reloc = obj->efile.reloc; @@ -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)
commit a5b8bd47dcc57 ("bpf tools: Collect eBPF programs from their own sections") 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: [root@p23lp27] # make V=1 EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2" [...] make -f /home6/tmricht/linux-next/tools/build/Makefile.build dir=./util/scripting-engines obj=libperf libbpf.c: In function ‘bpf_object__elf_collect’: libbpf.c:811:15: error: ignoring return value of ‘strerror_r’, declared with attribute warn_unused_result [-Werror=unused-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 target 'libbpf.o' failed 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(). Also fixes a possible initialization issue. 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(-)