Skip to content

Do kprobe.multi on addresses instead of symbols#297

Merged
brb merged 3 commits intocilium:mainfrom
Asphaltt:fix/unsupported_func
Feb 16, 2024
Merged

Do kprobe.multi on addresses instead of symbols#297
brb merged 3 commits intocilium:mainfrom
Asphaltt:fix/unsupported_func

Conversation

@Asphaltt
Copy link
Contributor

@Asphaltt Asphaltt commented Dec 15, 2023 •
edited
Loading

Fix #284

If we do kprobe.multi, kprobe on functions' addresses instead of symbols.

@Asphaltt Asphaltt changed the title Skip unsopported kprobe function Skip unsupported kprobe function Dec 15, 2023
@Asphaltt Asphaltt force-pushed the fix/unsupported_func branch 7 times, most recently from 6046b71 to 02c47e7 Compare December 16, 2023 15:08
@Asphaltt
Copy link
Contributor Author

Asphaltt commented Dec 17, 2023

By checking the CI log of bpf-next-main, we can see the unsupported kprobe functions are:

2023-12-17T11:30:32.2516205Z + grep 'Skip unsupported kprobe func' /tmp/pwru-advanced-ipv4.status
2023-12-17T11:30:32.2530356Z 2023/12/17 11:30:05 Skip unsupported kprobe func: e1000_xmit_frame
2023-12-17T11:30:32.2531623Z 2023/12/17 11:30:05 Skip unsupported kprobe func: calipso_skbuff_setattr
2023-12-17T11:30:32.2532906Z 2023/12/17 11:30:05 Skip unsupported kprobe func: calipso_skbuff_delattr
2023-12-17T11:30:32.2533677Z 2023/12/17 11:30:05 Skip unsupported kprobe func: add_grhead
2023-12-17T11:30:32.2534554Z 2023/12/17 11:30:05 Skip unsupported kprobe func: help

@Asphaltt Asphaltt force-pushed the fix/unsupported_func branch 8 times, most recently from a47465f to 0bd3a6b Compare December 19, 2023 05:18
@brb
Copy link
Member

brb commented Dec 19, 2023

Thanks for the PR!

I think the long term solution for both kprobe and kprobe.multi is to use func addrs instead of symbols. This would require extending the kprobe attach method in the cilium/ebpf library (should be trivial though). Meanwhile, it is already supported for the kprobe.multi.

As a temporary solution we can definitely accept this PR for the kprobe backend. However, I think for the kprobe.multi it might be too slow - the binary search tries to attach kprobes multiple times.

@Asphaltt
Copy link
Contributor Author

Asphaltt commented Dec 19, 2023

Meanwhile, it is already supported for the kprobe.multi.

Great. I'm on it.

@Asphaltt Asphaltt force-pushed the fix/unsupported_func branch from 0bd3a6b to 43cc299 Compare December 20, 2023 14:43
@Asphaltt Asphaltt changed the title Skip unsupported kprobe function Do kprobe.multi on addresses instead of symbols Dec 20, 2023
@Asphaltt Asphaltt force-pushed the fix/unsupported_func branch from fa5abbe to 8bc3128 Compare December 20, 2023 16:16
brb
brb reviewed Dec 20, 2023
View reviewed changes
@Asphaltt Asphaltt force-pushed the fix/unsupported_func branch from 8bc3128 to 6a20b3f Compare December 21, 2023 04:11
@brb
Copy link
Member

brb commented Feb 14, 2024

@Asphaltt 👋 I'm catching up on reviews. Is this PR ready for the review?

@Asphaltt
Refactor code of getting kprobe_skb bpf prog
c2a2c77 
With collection, we are able to get kprobe_skb bpf prog dynamically. So,
we can reduce some plain code.

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
@Asphaltt
Do kprobe.multi on addresses instead of symbols
c89f241 
As Martynas P. suggests, it's better to use func addrs instead of symbols
for `kprobe.multi`.

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
@Asphaltt
Revert "gh/workflows: Use the kprobe backend"
98c24dc 
As pwru runs kprobe.multi with function's addresses, we can revert
`--backend kprobe` to run with kprobe.multi.

This reverts commit c3a9091.
@Asphaltt Asphaltt force-pushed the fix/unsupported_func branch from 6a20b3f to 98c24dc Compare February 14, 2024 16:42
@Asphaltt
Copy link
Contributor Author

Asphaltt commented Feb 14, 2024

Yes, it's ready.

brb
brb approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will merge once I fix #323

@brb brb merged commit 724fd83 into cilium:main Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brb brb brb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Failing to load on bpf-next: calipso_skbuff_delattr: opening perf event: cannot assign requested address

2 participants

@Asphaltt @brb