Add initial CO:RE support #759

grantseltzer · 2021-06-09T20:58:01Z

This changes the way that tracee reads kernel data structures
so that:

If a compile time 'CORE' flag is specified, the bpf_core_read
helper is used which takes into account btf information extracted
from the running kernel.
KERN_READ calls are not embedded in one another. This was an
issue because the attribute that the BTF helper requires cannot
be embedded in one another.

This also adds supporting artifacts in the vmlinux.h file which
contains baseline definitions of all kernel structures, and a
header file which contains all macros tracee uses that aren't
defined in the vmlinux.h file.

Note: This PR does not address how we handle packaging of a CORE enabled tracee binary, nor does it handle testing infra.

Signed-off-by: grantseltzer grantseltzer@gmail.com

yanivagman

Great!
This is something we are very much looking forward to.

Regarding the vmlinux.h file - this file is huge! Do we have to include it as part of the repo? If yes, will we need to ever change it in the future as newer kernels will be introduced?

Also, have you verified that you code works for non-core compilation in kernel 4.18?

yanivagman · 2021-06-10T06:36:12Z

tracee-ebpf/Makefile

@@ -19,7 +19,7 @@ OUT_DIR ?= dist
 GO_SRC := $(shell find . -type f -name '*.go')
 OUT_BIN := $(OUT_DIR)/tracee-ebpf
 BPF_SRC := tracee/tracee.bpf.c 
-OUT_BPF := $(OUT_DIR)/tracee.bpf.$(subst .,_,$(KERN_RELEASE)).$(subst .,_,$(VERSION)).o
+OUT_BPF := $(OUT_DIR)/tracee.bpf.o


I think that we should keep this line for non CO-RE compilations. It makes sure that the compiled bpf code matches the kernel version and tracee userspace version.
For CO-RE compilation, we should probably remove the KERN_RELEASE part but keep the VERSION part.

Sounds good. I will pass a compile time variable into the Go code so that it knows what name to look for. I don't think having 'VERSION' is important for the co:re object, how about some indication that it's co:re?

tracee-ebpf/Makefile

yanivagman · 2021-06-10T06:48:34Z

tracee-ebpf/tracee/missing_macros.h

+/*
+ * The purpose of this file is to define macros
+ * that tracee.bpf.c relies on which are defined
+ * in linux kernel headers but not in vmlinux.h 


So from now on, every macro that we add to tracee.bpf.c should be added to here as well, right?
We should add to the pipeline CO-RE compilation so that we know that we didn't miss adding a macro to this file when needed.

Yes, and good point!

yanivagman · 2021-06-10T06:55:35Z

tracee-ebpf/tracee/missing_macros.h

+
+#define VM_READ		0x00000001	/* currently active flags */
+#define VM_WRITE	0x00000002
+#define VM_EXEC		0x00000004


Need to correct indentations in some places in this file

Tabs are a scourge

yanivagman · 2021-06-10T07:06:48Z

tracee-ebpf/tracee/tracee.bpf.c

 #define READ_KERN(ptr) ({ typeof(ptr) _val;                             \
                          __builtin_memset(&_val, 0, sizeof(_val));     \
                          bpf_probe_read(&_val, sizeof(_val), &ptr);    \
                          _val;                                         \
                        })
+#else
+// Try using READ_KERN here, just don't embed them in each other


As we previously discussed, maybe we can change this macro to get the arguments in a comma separated list (which is also supported by the BPF_CORE_READ and BPF_PROBE_READ macros given by libbpf)

Agreed, this requires a little rewiring but I can do it.

Do we plan to do this one in this PR or do you want to finish it in a separate PR?

I'll take a crack at it for this PR

yanivagman · 2021-06-10T07:09:02Z

tracee-ebpf/tracee/tracee.bpf.c

@@ -256,7 +278,7 @@ struct bpf_map_def SEC("maps") _name = { \

 /*=============================== INTERNAL STRUCTS ===========================*/

-typedef struct context {
+typedef struct internal_proc_context {


Any reason for changing this name?
It is not only the proc context that is in this struct

There's a name collision with the enum context in vmlinux.h. I'll change it to 'internal_context'.

So maybe "event_context"?

yanivagman · 2021-06-10T07:12:49Z

tracee-ebpf/tracee/tracee.bpf.c

+                bpf_probe_read(&dentry, sizeof(struct dentry*), &mnt_p->mnt_mountpoint);
+                bpf_probe_read(&mnt_p, sizeof(struct mount*), &mnt_p->mnt_parent);
+                bpf_probe_read(&mnt_parent_p, sizeof(struct mount*), &mnt_p->mnt_parent);
+                bpf_probe_read(&vfsmnt, sizeof(struct vfsmount*), (const void*)&mnt_p->mnt);


Weren't these changes already part of your previously commit? rebase?

yanivagman · 2021-06-10T07:16:09Z

tracee-ebpf/tracee/tracee.bpf.c

    struct ipv6_pinfo *np = inet6_sk_own_impl(sk, inet);

    struct in6_addr addr = {};
    addr = get_sock_v6_rcv_saddr(sk);
-    if (ipv6_addr_any(&addr)){
+    if (ipv6_any_addr(&addr)){


This function, and inet_sk as well were part of the headers which are now removed for CO-RE, right?
I think that it would be better to put them in the "missing_macros" file (rename?), which should compensate for all the missing things that we had in the headers

yanivagman · 2021-06-10T07:18:11Z

tracee-ebpf/tracee/tracee.bpf.c

@@ -1675,7 +1737,7 @@ int tracepoint__raw_syscalls__sys_enter(struct bpf_raw_tracepoint_args *ctx)
    int id = ctx->args[1];
    struct task_struct *task = (struct task_struct *)bpf_get_current_task();

-#if defined(CONFIG_ARCH_HAS_SYSCALL_WRAPPER)
+if (CONFIG_ARCH_HAS_SYSCALL_WRAPPER) {


What about the case that CORE is not defined? Does it work?
For x86 arch with kernels >= 4.18 this might not be a problem as this macro is always defined, but for ARM64 this is not case (added in a later kernel version)

As far as I can tell this causes no issue, but I don't have somewhere to test on ARM64?

This was added in kernel 4.17 for x86 and 4.20 for ARM64.
As tracee only supports kernels > 4.18, this should be tested on an ARM64 platform with a 4.18/4.19 kernel.
As we don't have any ARM64 platform with these kernels installed to test on, we can keep your changes and address any potential bug that might appear in the future.

grantseltzer · 2021-06-11T19:48:59Z

I'd like to have the vmlinux.h file as a backup in case the build machine doesn't have the vmlinux file. It shouldn't make a difference in terms of functionality. For example, I don't think we have access to it in our builder container.

yanivagman · 2021-06-14T14:15:39Z

tracee-ebpf/Makefile

 OUT_BPF := $(OUT_DIR)/tracee.bpf.$(subst .,_,$(KERN_RELEASE)).$(subst .,_,$(VERSION)).o
+else
+OUT_BPF := $(OUT_DIR)/tracee.bpf.core.o


This should probably be
$(OUT_DIR)/tracee.bpf.core.$(subst .,_,$(VERSION)).o
so that we can check if the bpf code matches the running tracee binary version (this might be relevant if someone upgrades tracee, but already have tracee.bpf.o already compiled to /tmp/tracee, for example)

yanivagman · 2021-06-14T14:19:12Z

tracee-ebpf/main.go

 	bpfObjFileName := fmt.Sprintf("tracee.bpf.%s.%s.o", strings.ReplaceAll(tracee.UnameRelease(), ".", "_"), strings.ReplaceAll(version, ".", "_"))
+	if bpf_core != "" {
+		bpfObjFileName = "tracee.bpf.o"


This should be
fmt.Sprintf("tracee.bpf.%s.o", strings.ReplaceAll(version, ".", "_"))
to match my comment in the makefile

yanivagman · 2021-06-14T14:29:37Z

tracee-ebpf/tracee/tracee.bpf.c

+#ifdef asm_inline
+#undef asm_inline
+#define asm_inline asm
+#endif


These lines are common for CO-RE and non-CORE compilations, we can probably remove them from here

yanivagman · 2021-06-14T14:30:27Z

tracee-ebpf/tracee/tracee.bpf.c

 #undef container_of
-//#include "bpf_core_read.h"
+#include "bpf_core_read.h"


Shouldn't we have brackets here as we have below?

yanivagman · 2021-06-14T14:57:05Z

tracee-ebpf/tracee/tracee.bpf.c

 #define READ_KERN(ptr) ({ typeof(ptr) _val;                             \
                          __builtin_memset(&_val, 0, sizeof(_val));     \
                          bpf_probe_read(&_val, sizeof(_val), &ptr);    \
                          _val;                                         \
                        })
+#else
+// Try using READ_KERN here, just don't embed them in each other


Do we plan to do this one in this PR or do you want to finish it in a separate PR?

yanivagman · 2021-06-14T15:06:51Z

tracee-ebpf/tracee/tracee.bpf.c

@@ -1675,7 +1737,7 @@ int tracepoint__raw_syscalls__sys_enter(struct bpf_raw_tracepoint_args *ctx)
    int id = ctx->args[1];
    struct task_struct *task = (struct task_struct *)bpf_get_current_task();

-#if defined(CONFIG_ARCH_HAS_SYSCALL_WRAPPER)
+if (CONFIG_ARCH_HAS_SYSCALL_WRAPPER) {


This was added in kernel 4.17 for x86 and 4.20 for ARM64.
As tracee only supports kernels > 4.18, this should be tested on an ARM64 platform with a 4.18/4.19 kernel.
As we don't have any ARM64 platform with these kernels installed to test on, we can keep your changes and address any potential bug that might appear in the future.

yanivagman · 2021-06-15T19:35:15Z

LGTM!

One of my last concerns is the missing macros file. What if some macros are changed between kernel versions? Also, what if these macros are not the same on different systems (for example PAGE_SIZE)?

I'm not sure if there are things that we can do to mitigate these problems, but we should be aware of them

grantseltzer · 2021-06-15T20:13:42Z

@yanivagman That's a good point, certainly worth the concern. Another way we can achieve CO:RE is to apply the 'preserve_access_index' attribute to all the headers we import in non-co:re variant of tracee-ebpf. Just like how it works in vmlinux.h:

#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)

That'd let us keep the macros from the headers but has its own sets of challenges.

grantseltzer · 2021-06-16T20:53:30Z

I got a bpf object that was built on a 5.12 kernel running on 4.18 with no issue.

I realized that we definitely should be documenting this as part of this PR. Additional things I want to make sure we make follow up issues for/document:

Tracee bpf code uses some data structure values that aren't present/renamed in various different kernel versions. This means that the binary likely runs perfectly fine on anything but doesn't build on older kernels. We should make sure we have compile flags in tracee.bpf.c for everything. Requires more investigation but is fine for now.
The vmlinux file, generating a fresh one for builds, etc...
Updating macros (see comment above)
Usage examples

yanivagman · 2021-06-20T04:49:21Z

I think that we should also try to tackle #776 errors in this PR, but not by generating vmlinux.h. Instead, we should probably wrap the existing kernel version ifdefs with an outer "ifndef CORE". WDYT?

Also, can you please rename vmlinux.h to vmlinux_512.h and put it in "x86" folder like done in https://github.com/iovisor/bcc/tree/master/libbpf-tools/x86 ?

Signed-off-by: grantseltzer <grantseltzer@gmail.com>

This changes the way that tracee reads kernel data structures so that: 1) If a compile time 'CORE' flag is specified, the bpf_core_read helper is used which takes into account btf information extracted from the running kernel. 2) KERN_READ calls are not embedded in one another. This was an issue because the attribute that the BTF helper requires cannot be embedded in one another. This also adds supporting artifacts in the vmlinux.h file which contains baseline definitions of all kernel structures, and a header file which contains all macros tracee uses that aren't defined in the vmlinux.h file. Signed-off-by: grantseltzer <grantseltzer@gmail.com>

itaysk · 2021-07-08T13:33:07Z

tracee-ebpf/Dockerfile.builder

-RUN echo "deb http://apt.llvm.org/buster/ llvm-toolchain-buster-9 main" >> /etc/apt/sources.list && apt-key adv --keyserver hkps://keyserver.ubuntu.com --recv-keys 15CF4D18AF4F7421 && \
-    DEBIAN_FRONTEND=noninteractive apt-get update -y && apt-get install -y --no-install-recommends libelf-dev llvm-9 clang-9 && \
-    (for tool in "clang" "llc" "llvm-strip"; do path=$(which $tool-9) && ln -s $path ${path%-*}; done)
+RUN echo "deb http://apt.llvm.org/buster/ llvm-toolchain-buster-12 main" >> /etc/apt/sources.list && apt-key adv --keyserver hkps://keyserver.ubuntu.com --recv-keys 15CF4D18AF4F7421 && \


update the docs to reflect that llvm 12 is now required

itaysk · 2021-07-08T13:59:30Z

.github/workflows/pr-ebpf.yaml

@@ -28,5 +28,7 @@ jobs:
          fi
      - name: Build
        run: make all DOCKER=1
+      - name: Build CO:RE
+        run: make all DOCKER=1 CORE=y


can we make the examples/scripts use consistent config values: either 1 or y

itaysk · 2021-07-08T14:01:02Z

docs/install/ebpf-compilation.md

+
+# eBPF CO:RE Compilation
+
+Tracee also can utilize CO:RE (Compile once, run everywhere) technology enabled by libbpf. With this enabled, you can compile the tracee bpf object file on one system, and run it on any kernel with BTF (BPF type format) enabled. To check if your kernel has BTF enabled, check for the `CONFIG_DEBUG_INFO_BTF` in your kernel config.


check for the CONFIG_DEBUG_INFO_BTF in your kernel config.

can we write the command to do that?
can we make sure Tracee-eBPF's main does that and prints a nice error?

grantseltzer added the area/ebpf label Jun 9, 2021

grantseltzer added this to the CORE milestone Jun 9, 2021

grantseltzer requested review from itaysk, simar7 and yanivagman June 9, 2021 20:58

This was linked to issues Jun 9, 2021

Conditionally replace bpf_probe_read* calls with BPF_CORE_READ #662

Closed

Generate/package a vmlinux.h file for shipping with tracee-ebpf CO:RE functionality #663

Closed

Create conditional compile flag in tracee.bpf.c for CO:RE #706

Closed

yanivagman reviewed Jun 10, 2021

View reviewed changes

grantseltzer force-pushed the c-o-r-e branch 14 times, most recently from 8af3ed6 to 59f4822 Compare June 11, 2021 19:42

yanivagman reviewed Jun 14, 2021

View reviewed changes

grantseltzer force-pushed the c-o-r-e branch 2 times, most recently from 98b6863 to 7f68f24 Compare June 15, 2021 18:01

grantseltzer force-pushed the c-o-r-e branch from 7f68f24 to 8c2d780 Compare June 16, 2021 15:42

grantseltzer force-pushed the c-o-r-e branch from 8c2d780 to 3c66235 Compare June 17, 2021 16:36

grantseltzer added 2 commits June 20, 2021 05:59

Add locked vmlinux.h file

2338cf8

Signed-off-by: grantseltzer <grantseltzer@gmail.com>

grantseltzer force-pushed the c-o-r-e branch 6 times, most recently from 38392d0 to c180569 Compare June 22, 2021 18:27

grantseltzer requested a review from yanivagman June 22, 2021 20:23

yanivagman approved these changes Jun 22, 2021

View reviewed changes

grantseltzer merged commit 4bf8ca6 into main Jun 22, 2021

yanivagman mentioned this pull request Jul 3, 2021

Convert BCC to libbpf+CO-RE #209

Closed

grantseltzer mentioned this pull request Jul 5, 2021

Add a step to CO:RE compilation attempting to generate a vmlinux.h file #776

Closed

itaysk reviewed Jul 8, 2021

View reviewed changes

grantseltzer deleted the c-o-r-e branch October 1, 2021 03:37

itaysk mentioned this pull request Nov 25, 2021

Tracee Docker Image: Clang version is too old #1156

Closed

rafaeldtinoco mentioned this pull request Apr 10, 2023

capabilities: drop capabilities by default #2924

Merged

3 tasks


		# eBPF CO:RE Compilation

		Tracee also can utilize CO:RE (Compile once, run everywhere) technology enabled by libbpf. With this enabled, you can compile the tracee bpf object file on one system, and run it on any kernel with BTF (BPF type format) enabled. To check if your kernel has BTF enabled, check for the `CONFIG_DEBUG_INFO_BTF` in your kernel config.

Add initial CO:RE support #759

Add initial CO:RE support #759

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!