Introduce two enhancements for func IP#468
Conversation
There was a problem hiding this comment.
I left a comment regarding platform specific bpf code, other than that I love the way you fetched rip in fentry.
Some minor(?) concerns:
- I fully understand the context because I read Chinese and we talked about this in private. Martynas (brb) please feel free to ask for more details if commit messages don't elaborate enough.
- Lack of test is constantly hurting us. I'm not 100% confident in whether this PR won't break anything, considering we already suffered certain symbol-address mapping issue #462. Again, this is not the problem of this PR's author, I'm just saying we (pwru maintainers) might want to prioritize some tasks like "pwru hardening"
ba8ccdc to
c5d2c31
Compare
There was a problem hiding this comment.
I have to confess I don't have enough knowledge to review arm64 part, nor does pwru ci test have any arm environment.
Overall it looks good, only some comments to the (amazing) work in correct_func_ip().
I'm not 100% convinced we should adjust pc in bpf, because we can easily achieve the same in userspace at lower cost in my opinion. For example, a possible, maybe also reliable way to adjust pc is:
func adjustPC(pc uint64) uint64 {
_, ok := kallsyms[pc]func adjustPC(pc uint64) uint64 {
if _, ok := kallsyms[pc]; ok {
return pc
}
if _, ok := kallsyms[pc-1]; ok {
return pc - 1
}
if _, ok := kallsyms[pc-5]; ok {
return pc -5
}
return 0
}But I do appreciate how you get pc without pt_regs in fentry, thank you very much.
I don't think so. In userspace, we have to do the same thing like However, it's better to detect |
c5d2c31 to
da1c3f5
Compare
jschwinger233
left a comment
There was a problem hiding this comment.
As this PR passed latest CI checks, x64 parts LGTM.
|
Let me add a CI arm64 runner next week, and then I will review the PR. Sorry for the delay! |
|
@Asphaltt Thanks. |
Since commit 55bdaac ("Fix tracing tc-bpf with --filter-track-skb-by-stackid"), we are able to get FP of current bpf prog. Then, from blog [Debug a tailcall BUG with fentry](https://asphaltt.github.io/post/ebpf-talk-138-debug-tailcall-bug-with-fentry/), we are able to get IP of tracee bpf prog like the way of `bpf_get_func_ip()` helper. Therefore, implement our own `get_func_ip()` helper to get IP of tracee bpf prog. Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
As we're able to get IP of tracee bpf prog at runtime, remove those code related to name2progName preparation. Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
It's better to correct func IP in bpf than in user space, because in bpf it is able to distinguish every case, include endbr case. As a result, it gets `addr` by unified `get_addr()` function for **kprobe**, **kprobe-multi**, **fentry** and **fexit**. Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
On arm64, R10 of bpf is not FP, aka A64_FP register. It should reuse `detect_tramp_fp()` function to get a valid FP. Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
499e7bf to
522cd27
Compare
I encountered the following error days ago.
Then, I realize that we have to improve getting func IP when trace bpf progs.
With commit 55bdaac ("Fix tracing tc-bpf with --filter-track-skb-by-stackid") and my blog Debug a tailcall BUG with fentry, I think it's able to get tracee IP when trace bpf progs.
Then, let me introduce our own
get_func_ip()helper for tracing to get tracee IP.Next, in 'output.go', it's incorrect to get func name when output in JSON format.
Why not correct func IP in bpf in order to make sure
event.addris correct always?To achieve it, let us do
- 1for kprobe and- 4for endbr in 'kprobe_pwru.c'. As a result, introduceget_addr()function for kprobe, kprobe-multi, fentry and fexit.BTW, clean some Go code that was used for tracing bpf progs.
get_func_ip()must be compatible on arm64Test on arm64:
Test on amd64: