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:   Fri, 27 Jul 2018 10:57:57 -0700
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Thomas-Mich Richter <tmricht@linux.ibm.com>
Cc:     Daniel Borkmann <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: <20180727105757.2d851920@cakuba.netronome.com>
In-Reply-To: <325e3d58-89fb-1b50-8865-22c6f372671a@linux.ibm.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>
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 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!