Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARM-specific vmlinux.h #786

Closed
kkourt opened this issue Mar 10, 2023 · 10 comments · Fixed by #3308
Closed

ARM-specific vmlinux.h #786

kkourt opened this issue Mar 10, 2023 · 10 comments · Fixed by #3308
Labels
arch/arm64 arm64 issues kind/enhancement This improves or streamlines existing functionality

Comments

@kkourt
Copy link
Contributor

kkourt commented Mar 10, 2023

#734 used the x86 vmlinux.h as a quick way to get the arm64 port going.

Long term, we might want to use a vmlinux.h generated from an arm64 kernel.
Specifically:

  • generate an arm vmlinux.h and place it in include/arm64/vmlinux.h
  • move the old (x86) version to include/x86/vmlinux.h
  • have a include/vmlinux.h that picks the proper file gbased on the ARCH

@mtardy tried this with https://gist.github.com/mtardy/ec17aa35132b4fc769519f166b808f5b, but it was not trivial to use.

@kkourt kkourt added the arch/arm64 arm64 issues label Mar 10, 2023
@mtardy
Copy link
Member

mtardy commented Mar 10, 2023

See this discussion also #734 (comment), @tixxdz mentioned that the vmlinux.h in its current state after the specified PR merged might be wrong for arm64. Indeed, some arch-specific values in enums might exist (and miss in arm64) which might result in very difficult-to-debug issues. For example, this enum value might miss on arm64 and shift all the values by 1.

@mtardy mtardy mentioned this issue Mar 13, 2023
@lmb
Copy link
Contributor

lmb commented Mar 20, 2023

FWIW, CO-RE will take care of changing enum values around, so the concern mentioned in the linked PR shouldn't be an issue. As long as you don't refer to x86 only enums in your BPF .c things should work (famous last words).

Fixing pt_regs itself is harder since there field names don't line up, but there was discussion on bpf@ how to fix this using CO-RE magic. Let me know if you want that dug up.

@tixxdz
Copy link
Member

tixxdz commented Mar 20, 2023

FWIW, CO-RE will take care of changing enum values around, so the concern mentioned in the linked PR shouldn't be an issue. As long as you don't refer to x86 only enums in your BPF .c things should work (famous last words).

Thanks @lmb for the comment, that's also part of the concern since tetragon allows to hook anywhere with yaml files turned into kprobes. We can't control what users do... I guess an arch specific flag enum can easily cause trouble and hard to track...

@lmb
Copy link
Contributor

lmb commented Mar 20, 2023

I guess an arch specific flag enum can easily cause trouble and hard to track...

Do you have an example in mind? The intended failure mode is that the program referring an arch specific enum that doesn't exist in the current kernel will fail to load with an error telling you which enum value is missing. If that is currently not the case I'd consider it a bug and something we should fix for sure!

@tixxdz
Copy link
Member

tixxdz commented Mar 22, 2023

Do you have an example in mind? The intended failure mode is that the program referring an arch specific enum that doesn't exist in the current kernel will fail to load with an error telling you which enum value is missing. If that is currently not the case I'd consider it a bug and something we should fix for sure!

Ok, that's great then if CO-RE can adapt or error out on it!

So for now seems we are fine, thanks @lmb ;-) ! what remains is minor: as you noted if we start using arm specific bpf programs then having arm specific values will help to compile, and always remember to look at kernel and not vmlinux.h as source of truth to do Tetragon matchArgs on raw integer values (not symbols) in tracing policies...

These are minor issues, we can fix later or maybe some one will improve it.

@kkourt
Copy link
Contributor Author

kkourt commented Mar 24, 2023

OK, so it seems to me that the conclusion is that we are not aware of any potential issues with how things are now. So I'd suggest we close this issue. An objections?

Thanks @lmb!

Fixing pt_regs itself is harder since there field names don't line up, but there was discussion on bpf@ how to fix this using CO-RE magic. Let me know if you want that dug up.

I'd be interested in reading those, if you can find them in 5' or so :)

@tixxdz
Copy link
Member

tixxdz commented Mar 24, 2023

OK, so it seems to me that the conclusion is that we are not aware of any potential issues with how things are now. So I'd suggest we close this issue. An objections?

Still there are arch specific x86 or arm only values issues for BPF .c , closing it means we resolved this, which is not 100% true see previous comments.

So we can close as it doesn't affect us right now, or be nice, keep it open mark it enhancement for future references and context. Maybe some one using Tetragon to do bpf on arm, will contribute it. I'm saying this since right now Tetragon is the only pratical useful engine where you can just stuck your BPF code and let it do the rest ;-) .

@lmb
Copy link
Contributor

lmb commented Mar 27, 2023

I'd be interested in reading those, if you can find them in 5' or so :)

Seems like something was actually merged: https://www.spinics.net/lists/bpf/msg64490.html cilium/ebpf currently doesn't support this but there is someone in the community looking at adding support if I understand correctly.

@mtardy mtardy added the kind/enhancement This improves or streamlines existing functionality label Mar 27, 2023
@kkourt
Copy link
Contributor Author

kkourt commented Mar 28, 2023

OK, so it seems to me that the conclusion is that we are not aware of any potential issues with how things are now. So I'd suggest we close this issue. An objections?

Still there are arch specific x86 or arm only values issues for BPF .c , closing it means we resolved this, which is not 100% true see previous comments.

My expectation was that validation would fail for those cases and the programs would never be loaded. Am I missing something?

@jrfastab
Copy link
Contributor

arm is working refile bugs as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch/arm64 arm64 issues kind/enhancement This improves or streamlines existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants