-
Notifications
You must be signed in to change notification settings - Fork 617
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
Fix issues when porting alloy/pyroscope to android #3582
Conversation
mingjun.zhou seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
ebpf/session.go
Outdated
if err != nil { | ||
return fmt.Errorf("pyrobpf rewrite constants %w", err) | ||
// if the file does not exist, CONFIG_PID_NS is not supported, so we just ignore the error | ||
if os.IsExist(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something looks off here. I would expect this condition to be true if error is ErrExist and I do not expect this error to be returned during pid ns check ( we do not create any files/objects).
The next error check looks suspicious as well. It looks like we rewrite global_config constant here only in case if ErrExist and err==nil which looks impossible to my brain
Did you test this change with PD_NS configured and inside a pid namespace ( a docker container for example)?
We need to change something here.
Let me know if I am missing/misunderstanding something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Updated. I'm new to golang and it's my misunderstanding of os.IsExist
. It's now replaced by !os.IsNotExist()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the new patch on WSL and Android Phone, both work fine.
@@ -94,5 +95,9 @@ func NewKallsymsFromData(kallsyms []byte) (*SymbolTab, error) { | |||
if allZeros { | |||
return NewSymbolTab(nil), nil | |||
} | |||
// kallsyms maybe unsorted when bpf/modules are loaded from userspace after kernel boot. | |||
sort.Slice(syms, func(i, j int) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It would be nice to add a test, but it is is not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Updated. I add 2 unordered symbols in testdata. I tested it and it will fail without this patch and success with this patch.
Thank you for the contribution. I left some comments. |
ed1c6eb
to
94ff4bc
Compare
94ff4bc
to
8935a42
Compare
Thank you for the contribution! |
I'm porting alloy/pyroscope to the mobile phone running Android 14. And I found some issues:
The config
CONFIG_PID_NS
is not set in standard Android kernel, which means /proc/self/ns/pid is not exist and alloy will exit with failure. The first commit fixed this issue by ignoring error when /proc/self/ns/pid doesn't exist.When kernel modules or bpf programs are dynamic loaded from userspace after kernel boot, the address in /proc/kallsyms may be unordered, which causes the origin code find wrong symbol. The second commit fixed this issue by sorting the symbol by address.
Here's an example for such /proc/kallsyms: