[PATCH bpf] tools: bpftool: fix compilation with older headers

[PATCH bpf] tools: bpftool: fix compilation with older headers

[Jiri Benc]

Compilation of bpftool on a distro that lacks eBPF support in the installed
kernel headers fails with:

common.c: In function ‘is_bpffs’:
common.c:96:40: error: ‘BPF_FS_MAGIC’ undeclared (first use in this function)
  return (unsigned long)st_fs.f_type == BPF_FS_MAGIC;
                                        ^
Fix this the same way it is already in tools/lib/bpf/libbpf.c and
tools/lib/api/fs/fs.c.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 tools/bpf/bpftool/common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 0b482c0070e0..465995281dcd 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -55,6 +55,10 @@
 
 #include "main.h"
 
+#ifndef BPF_FS_MAGIC
+#define BPF_FS_MAGIC		0xcafe4a11
+#endif
+
 void p_err(const char *fmt, ...)
 {
 	va_list ap;
-- 
1.8.3.1

Re: [PATCH bpf] tools: bpftool: fix compilation with older headers

[Daniel Borkmann]

On 03/06/2018 02:50 PM, Jiri Benc wrote:
> Compilation of bpftool on a distro that lacks eBPF support in the installed
> kernel headers fails with:
> 
> common.c: In function ‘is_bpffs’:
> common.c:96:40: error: ‘BPF_FS_MAGIC’ undeclared (first use in this function)
>   return (unsigned long)st_fs.f_type == BPF_FS_MAGIC;
>                                         ^
> Fix this the same way it is already in tools/lib/bpf/libbpf.c and
> tools/lib/api/fs/fs.c.
> 
> Signed-off-by: Jiri Benc <jbenc@redhat.com>

Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under
tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h.

Both bpftool and libbpf have tools/include/uapi/ in their include path from their
Makefile, so they would pull this in automatically and it would also allow to get rid
of the extra ifdef in libbpf then. Could you look into that?

Thanks,
Daniel

> ---
>  tools/bpf/bpftool/common.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 0b482c0070e0..465995281dcd 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -55,6 +55,10 @@
>  
>  #include "main.h"
>  
> +#ifndef BPF_FS_MAGIC
> +#define BPF_FS_MAGIC		0xcafe4a11
> +#endif
> +
>  void p_err(const char *fmt, ...)
>  {
>  	va_list ap;
> 

Re: [PATCH bpf] tools: bpftool: fix compilation with older headers

[Jiri Benc]

On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote:
> Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under
> tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h.
> 
> Both bpftool and libbpf have tools/include/uapi/ in their include path from their
> Makefile, so they would pull this in automatically and it would also allow to get rid
> of the extra ifdef in libbpf then. Could you look into that?

That's what I tried at first. But honestly, this is a shortcut to hell.
Eventually, we'd end up with most of uapi headers duplicated under
tools/include/uapi and hopelessly out of sync.

The right approach would be to export uapi headers from the currently
built kernel somewhere (a temporary directory, perhaps) and use that to
build the tools. We should not have duplicated and out of sync headers
in the kernel tree. Just look at the git log for tools/include/uapi to
see what I mean by "out of sync".

But that's certainly not a fix suitable for bpf.git, while I think the
build problem should be fixed in bpf.git. We can do something more
clever in bpf-next.

I can of course add the magic.h header if you have strong opinion about
that. I just don't think it's the right approach and seeing the
precedent in tools/lib/bpf/libbpf.c and tools/lib/api/fs/fs.c, I think
this minimal patch is better at this point until the duplicate headers
mess is sorted out.

Please let me know if you really want me to duplicate the magic.h file.

Thanks,

 Jiri

Re: [PATCH bpf] tools: bpftool: fix compilation with older headers

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Tue, 6 Mar 2018 16:03:25 +0100

> On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote:
>> Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under
>> tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h.
>> 
>> Both bpftool and libbpf have tools/include/uapi/ in their include path from their
>> Makefile, so they would pull this in automatically and it would also allow to get rid
>> of the extra ifdef in libbpf then. Could you look into that?
> 
> That's what I tried at first. But honestly, this is a shortcut to hell.
> Eventually, we'd end up with most of uapi headers duplicated under
> tools/include/uapi and hopelessly out of sync.
> 
> The right approach would be to export uapi headers from the currently
> built kernel somewhere (a temporary directory, perhaps) and use that to
> build the tools. We should not have duplicated and out of sync headers
> in the kernel tree. Just look at the git log for tools/include/uapi to
> see what I mean by "out of sync".

I understand your frustration.

I'm really puzzled why doing "make headers_install" and then building
these tools does not pick those in-kernel headers up.  That's what
really should happen.

The kernel tree internally should be self-consistent.

It's one thing for an external tool like iproute2 to duplicate stuff
like this, but user programs inside the kernel have no excuse for
requiring things like that just to build.

Re: [PATCH bpf] tools: bpftool: fix compilation with older headers

[Daniel Borkmann]

[ +acme ]

On 03/06/2018 05:00 PM, David Miller wrote:
> From: Jiri Benc <jbenc@redhat.com>
> Date: Tue, 6 Mar 2018 16:03:25 +0100
> 
>> On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote:
>>> Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under
>>> tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h.
>>>
>>> Both bpftool and libbpf have tools/include/uapi/ in their include path from their
>>> Makefile, so they would pull this in automatically and it would also allow to get rid
>>> of the extra ifdef in libbpf then. Could you look into that?
>>
>> That's what I tried at first. But honestly, this is a shortcut to hell.
>> Eventually, we'd end up with most of uapi headers duplicated under
>> tools/include/uapi and hopelessly out of sync.
>>
>> The right approach would be to export uapi headers from the currently
>> built kernel somewhere (a temporary directory, perhaps) and use that to
>> build the tools. We should not have duplicated and out of sync headers
>> in the kernel tree. Just look at the git log for tools/include/uapi to
>> see what I mean by "out of sync".
> 
> I understand your frustration.
> 
> I'm really puzzled why doing "make headers_install" and then building
> these tools does not pick those in-kernel headers up.  That's what
> really should happen.

Arnaldo, given this came out of tools/perf originally and duplicating/syncing
headers is common practice since about 2014 in kernel git tree, do you have
some context on why the above was/is not considered?

My current understanding is that the general preference would be on copying
the headers into tools/include/ infrastructure once there are dependencies
identified that would be missing on older/local system headers rather than
ifdef'ery of various bit and pieces in the code that need to make use of them.
Would be good to get some clarification on that in any case.

But that said, I'd also be fine taking the three-liner as is into bpf as a fix.

> The kernel tree internally should be self-consistent.
> 
> It's one thing for an external tool like iproute2 to duplicate stuff
> like this, but user programs inside the kernel have no excuse for
> requiring things like that just to build.

Re: [PATCH bpf] tools: bpftool: fix compilation with older headers

[Arnaldo Carvalho de Melo]

[ +ingo, jolsa, namhyung ]

Em Tue, Mar 06, 2018 at 05:28:33PM +0100, Daniel Borkmann escreveu:
> [ +acme ]
> 
> On 03/06/2018 05:00 PM, David Miller wrote:
> > From: Jiri Benc <jbenc@redhat.com>
> > Date: Tue, 6 Mar 2018 16:03:25 +0100
> > 
> >> On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote:
> >>> Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under
> >>> tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h.
> >>>
> >>> Both bpftool and libbpf have tools/include/uapi/ in their include path from their
> >>> Makefile, so they would pull this in automatically and it would also allow to get rid
> >>> of the extra ifdef in libbpf then. Could you look into that?
> >>
> >> That's what I tried at first. But honestly, this is a shortcut to hell.
> >> Eventually, we'd end up with most of uapi headers duplicated under
> >> tools/include/uapi and hopelessly out of sync.
> >>
> >> The right approach would be to export uapi headers from the currently
> >> built kernel somewhere (a temporary directory, perhaps) and use that to
> >> build the tools. We should not have duplicated and out of sync headers
> >> in the kernel tree. Just look at the git log for tools/include/uapi to
> >> see what I mean by "out of sync".
> > 
> > I understand your frustration.
> > 
> > I'm really puzzled why doing "make headers_install" and then building
> > these tools does not pick those in-kernel headers up.  That's what
> > really should happen.
> 
> Arnaldo, given this came out of tools/perf originally and duplicating/syncing
> headers is common practice since about 2014 in kernel git tree, do you have
> some context on why the above was/is not considered?

So, when tools/perf/ started we tried to use kernel code directly,
things like rbtree, list.h, etc.

It worked, we were sharing stuff with the kernel, all is well.

Then someone changes something in one of these files and tools/
compilation broke.

Fine for tools/ developers, we knew something like that could happen and
would fix things in tools/, life goes on.

Then Linus, IIRC, tried building tools/perf/ when something like that
had happened and the build broke for him.

He didn't liked that and we came up with this copy and check thing: we
copy something into tools/include/ and add it to
tools/perf/check-headers.sh, when something changes in the kernel,
nothing breaks, we get notified, check if the change implies changes in
tools/perf/, things like improving 'perf trace' to deal with new ioctl
or syscall flags, new syscalls, etc. Sometimes the copy ends up
automatically updating the tools, as there are scripts that generate
ioctl id->string tables, for instance, automatically from the updated
headers, things like what happened in this header update:

    http://git.kernel.org/acme/c/1350fb7d1b48
    tools include powerpc: Grab a copy of arch/powerpc/include/uapi/asm/unistd.h

With this in place no kernel developer needs to care about what happens
in tools/, tools/ developers don't need to worry about getting in the
way of kernel developers day-to-day activities.

Then we also have:

[acme@jouet perf]$ make help | grep perf
  perf-tar-src-pkg    - Build perf-4.16.0-rc4.tar source tarball
  perf-targz-src-pkg  - Build perf-4.16.0-rc4.tar.gz source tarball
  perf-tarbz2-src-pkg - Build perf-4.16.0-rc4.tar.bz2 source tarball
  perf-tarxz-src-pkg  - Build perf-4.16.0-rc4.tar.xz source tarball
[acme@jouet perf]$ 

Use that, get the resulting tarball, and all you need to build it
anywhere should be self contained there, so the tools may use flags,
defines, syscall definitions, etc, without ifdefs, and the resulting
source code will build in many places, cross compiling, etc, like is
done for every pull request I send to Ingo, see for instance the 53
containers where this is all built in a pull req like the one from
yesterday:

  http://lkml.kernel.org/r/20180305142932.16921-1-acme@kernel.org

But now there are more tools in tools/ and of course we can and should
improve the whole process in a way that satisfies the various projects.

So, with this said, I'll try and read the above thread.

Ingo may add some other thoughts here, this is what came to my mind now.

- Arnaldo
 
> My current understanding is that the general preference would be on copying
> the headers into tools/include/ infrastructure once there are dependencies
> identified that would be missing on older/local system headers rather than
> ifdef'ery of various bit and pieces in the code that need to make use of them.
> Would be good to get some clarification on that in any case.
> 
> But that said, I'd also be fine taking the three-liner as is into bpf as a fix.
> 
> > The kernel tree internally should be self-consistent.
> > 
> > It's one thing for an external tool like iproute2 to duplicate stuff
> > like this, but user programs inside the kernel have no excuse for
> > requiring things like that just to build.

Re: [PATCH bpf] tools: bpftool: fix compilation with older headers

[Daniel Borkmann]

On 03/06/2018 02:50 PM, Jiri Benc wrote:
> Compilation of bpftool on a distro that lacks eBPF support in the installed
> kernel headers fails with:
> 
> common.c: In function ‘is_bpffs’:
> common.c:96:40: error: ‘BPF_FS_MAGIC’ undeclared (first use in this function)
>   return (unsigned long)st_fs.f_type == BPF_FS_MAGIC;
>                                         ^
> Fix this the same way it is already in tools/lib/bpf/libbpf.c and
> tools/lib/api/fs/fs.c.
> 
> Signed-off-by: Jiri Benc <jbenc@redhat.com>

Applied it to bpf tree, thanks Jiri!

If preferred by Arnaldo/Ingo/others we can still pull in magic.h later on in
bpf-next and get rid of the ifdefs, not a big thing either.