Date:   Fri, 27 Jul 2018 09:22:03 +0200
From:   Thomas-Mich Richter <tmricht@...ux.ibm.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     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 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.


-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Powered by blists - more mailing lists