tools: bpftool: fix compilation with older headers

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 <redacted>
---
 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

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 <redacted>

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

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

From: Jiri Benc <redacted>
Date: Tue, 6 Mar 2018 16:03:25 +0100

On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote:

quoted

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.

[ +acme ]

On 03/06/2018 05:00 PM, David Miller wrote:

From: Jiri Benc <redacted>
Date: Tue, 6 Mar 2018 16:03:25 +0100

quoted

On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote:

quoted

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.

[ +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:

quoted

From: Jiri Benc <redacted>
Date: Tue, 6 Mar 2018 16:03:25 +0100

quoted

On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote:

quoted

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]$ 

20 more lines

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.

quoted

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.

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 <redacted>

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.