bpf: replace arch-specific pt_regs assumptions with bpf co-re macros

[Forsworns]

1. add libbpf header bpf_tracing.h
2. replace probe_read to CORE macros on pt_regs
3. add a new configuration item in bpf/Makefile to assign arch
4. revise the definition __VMLINUX_ in vmlinux.h to __VMLINUX_H__, which is the standard definition generated via bpftool

Signed-off-by: Peihao Yang [email protected]

[jrfastab]

Take a look at this https://elixir.bootlin.com/linux/latest/source/tools/lib/bpf/bpf_tracing.h#L140 it shows the differences between the targets.

[Forsworns]

@jrfastab I'm not sure if this is what you want.

The original probe_read on pt_regs are replaced by PT_REGS_*_CORE and PT_REGS_*_CORE_SYSCALL.

But now we have to declare the ARCH before make (see changes in bpf/Makefile). By default, it's x86.
Moreover, we have to regenerate the vmlinux.h for the specific arch, e.g., arm64.

[Forsworns]

There are some "Missing or malformed SPDX-License-Identifier tag in line 1" errors in the Check pull request commits. But I don't know which license should be added to these headers. :)

(Now, they are set to /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */ to pass the CI... )

[jrfastab]

Thanks @Forsworns I'll review this tomorrow.

[Forsworns]

The CI checkpatch provides some suggestions on bpf_tracing.h, so it fails.

But they are not applicable, e.g. CHECK:MACRO_ARG_REUSE: Macro argument reuse 'name' - possible side-effects? for the BPF_KPROBE

Maybe we can simply neglect them...

[Forsworns]

@jrfastab Feel free to discard the second commit if you think we don't need BPF_KPROBE and BPF_KRETPROBE.

[tpapagian]

Hey @Forsworns, let me provide some thoughts on this.

First, regarding the header, instead of /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */ please use:

// SPDX-License-Identifier: GPL-2.0
/* Copyright Authors of Cilium */

similar to what other files have (i.e. bpf/process/bpf_generic_kprobe.c).

Regarding bpf_tracing.h, I have seen that you made changes there described in 6af3f7f. I propose to get a copy of bpf_tracing.h from https://github.com/libbpf/libbpf/blob/master/src/bpf_tracing.h without any changes at all (also no formatting; you can exclude that here and here). Even the definition of __always_inline can be on another file.

In the commit message you also mention: revise kprobes' signature with co-re macros. I believe that there is no need to do that. The *_CORE variants are required only for some tracepoints (check here and here). Here you use them only in kprobes.

The reason that I am saying that is that we don't introduce bpf_tracing.h only for these cases. There are several macros there that can be used in other places as well (like new features) and I think that it is not a good idea to have different semantics compared to the original file.

Finally, the use of these macros in the other files, seems fine to me.

[Forsworns]

Thanks! @tpapagian Most of your comments are valuable to me

But here are two points:

---

I propose to get a copy of bpf_tracing.h without any changes at all

I revised the bpf_tracing.h mainly to pass the compilation.

---

In the second commit, I mentioned that revise kprobes' signature with co-re macros. This in fact follows your advices in

Assuming we have BPF_KPROBE macro we can make change the definition of this function and remove entirely this like.

Maybe my commit descriptions mislead you.

And in your new comment

The *_CORE variants are required only for some tracepoints.

This is not right. Just as shown in the link you provided, BPF_KROPE is indeed used to hide *_CORE calls.

+SEC("kprobe/hrtimer_nanosleep")
+int BPF_KPROBE(handle__kprobe,
+	       ktime_t rqtp, enum hrtimer_mode mode, clockid_t clockid)
+{
+	if (rqtp == MY_TV_NSEC)
+		kprobe_called = true;
+	return 0;
+}

[tpapagian]

Sorry @Forsworns , I maybe misread the comment in you commit. I created a diff of this and this but still there are some differences.

For example this and this are missing from the file that you have submitted. I can understand that maybe we don't care about these but I am just trying to understand why there are differences (do we need the in order to compile?). If they are needed in order to compile we are fine.

[Forsworns]

For example this and this are missing from the file that you have submitted. I can understand that maybe we don't care about these but I am just trying to understand why there are differences (do we need the in order to compile?). If they are needed in order to compile we are fine.

The first one, yes. I missed this architecture. Maybe I deleted it unconsciously... I forget it. It has been added.

The second one, we don't need it, since our target (indicated by Clang) will always be bpf. See this line in Makefile.

[Forsworns]

I replace the license in the generic_calls.h and retprobe_map.h to

// SPDX-License-Identifier: GPL-2.0
/* Copyright Authors of Cilium */

as you mentioned. But I guess bpf_tracing.h should be kept the same as other Linux headers, since it is copied from the libbpf library, e.g., the

tetragon/bpf/libbpf/bpf_core_read.h

Line 1 in 4bd5619

/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */

[tpapagian]

I believe that now it's good to be approved. Thanks for addressing my comments.

[olsajiri]

so what's your usecase, do you need tetragon to be built on another arch or you need CORE functionality? I think we should support both, but I'm curious how you will use the change

[Forsworns]

so what's your usecase, do you need tetragon to be built on another arch or you need CORE functionality?

@olsajiri

In fact, it's related to the issue #190

I didn't directly close the issue with this PR since it also related to a CI lint job as mentioned by willfindlay in #190.

[Forsworns]

I reset this PR and re-split the committed changes.

[kkourt]

@Forsworns Thanks! Some of the comments from @olsajiri are marked as resolved but it's not clear to me how they were actually resolved. Could you please reply in each of them how they were resolved?

[Forsworns]

@kkourt Some of them are solved after reorganizing the original two commits.

Because I once was not sure whether following advice from @tpapagian or not. So @olsajiri felt confused that part of "unnecessary" codes are supplemented in the original second commit.

The discussion has been too long, I paste the @tpapagian's advice here again.

Regarding bpf_tracing.h, I have seen that you made changes there described in 6af3f7f. I propose to get a copy of bpf_tracing.h from https://github.com/libbpf/libbpf/blob/master/src/bpf_tracing.h without any changes at all (also no formatting; you can exclude that here and here). Even the definition of __always_inline can be on another file.

[Forsworns]

@kkourt feel free to reopen the comments you feel not resolved

[kkourt]

Thanks for adding the comments!

I'd like to have an ACK from @olsajiri before we merge this, and I think a response on how each issue was resolved will help this.

[olsajiri]

looks good, added few nits

[Forsworns]

Rebased and now it passes the go tests :)

[olsajiri]

@Forsworns could you please rebase to the latest main, it might be the reason for the 'generated files check' fail,
other than that the change looks good to me

thanks

[Forsworns]

@olsajiri Have rebased :)

[kkourt]

Thanks!

PS: checkpatch issues seem to be a file found in libbpf,. so it does not make sense to address them.

[tixxdz]

@Forsworns thank you!

Maybe it makes sense to add a checkpatch rule to catch future direct registers usage...

cc @kkourt