-
Notifications
You must be signed in to change notification settings - Fork 165
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
docs(proposals): proposal for a new bpf probe #268
Conversation
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Hi @Andreagit97. Thanks for your PR. I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This is incredibly cool, man! Thank you very much! Thank you, hopefully we'll get our hands dirty on this asap :) |
This is beautiful. Well done. I have some questions on specifically what the embedded bytecode does. However at a glance this looks like a very persuasive proposal. You have my full support for pushing this through. The For Falco sustainability and reliability I would encourage us to use this via a feature gate on day one. Ideally this new probe is something that users can opt in to as needed. I would not like to release a new version of Falco that only has this code available. So maybe it would be better to frame this as an "alternative" instead of a hard "replacement"? |
Hi @kris-nova, thank you very much for your support! I completely agree with you: this probe is just an alternative to the current one. Users would choose according to their kernel versions and specific needs. For what concerns the bytecode, I will try to explain a little bit here. |
Very cool proposal indeed! I just wanted to add there is the option when using BTF probes to provide a path to a custom file holding this information, which could be generated for kernels that were compiled without BTF support as well. The implications might be more than what I understand, but I think this means we could eventually use this new probe with kernels older than 5.8, the raw tracepoint would need to be used in that case but everything else might just fall into place. Maybe not something to address right now, but it could be a way to re-unify the existing and new probe designs if it ever comes to that. It's also important to note that loading incorrect BTF information could result in some unwanted behavior, so that needs to be handled with care. |
Hi @Molter73, nice to see you again here 😄 I think we have to split this discussion into three main topics: 1. The target kernel version.The main goal behind this new probe is to use emerging tracing technologies to improve performance and usability. IMHO, there are 3 key points for reaching this:
As you can notice, with kernels older than 2. What we could backport to the current probe
These are definitely important points but more from a developer perspective than from a user one, so, as a first step, I would focus my attention on what could really bring us a huge value: having a more performant and modular probe that is easier to understand and to contribute. After that, I completely agree with you: we can try to resue and adapt what we can in our current probe. IMHO the rationale here is: as a first step, let's try to reach something working, keeping our current probe as a lifebelt. As soon as we have something satisfying and quite stable, we could start the porting phase. 3. What we can do with BTF information.Right now, we can use BTF information for two main scopes:
As you correctly pointed out in your comment,
The problem here is that this direct access must be granted directly by the verifier and not by Let me know if these thoughts sound good to you :) |
Thanks for the detailed explanation @Andreagit97! I must admit I'm not as well versed in eBPF as I'd like to and appreciate any bit of information I can get to learn more. My main concern with a new probe being created is that this will add an extra overhead to the development of new features. Things like supporting new syscalls already require developers to implement them both in the kernel module and eBPF probe, after this new probe is created the requirement of a third implementation might be needed as well for the new BTF enabled probe, so I'm trying to see if there's any way we could cut down on development effort without leaving anyone behind. |
Hi @Molter73 , I well understand your concerns, I thought a lot about this question. Unfortunately, we cannot avoid reimplementing our fillers since the underlying technologies are different. IMHO having a unique shared codebase for the bpf code would be a nightmare to maintain and would also decrease the actual performance since we should add some code to manage this duality. At the end of the day, the answer could be simply "yes", for a certain period of time we would probably need to add a new syscall for all our drivers. But here can come into play what we have said in the previous comment: when we will have something working with the new probe, we could backport the new structure to the current probe obtaining a sort of common pattern to manage events. The idea is to reach something like this also in the current probe: int BPF_PROG(ppme_syscall_open_x,
struct pt_regs *regs,
long ret)
{
/* Get per-CPU auxiliary map. */
struct auxiliary_map *aux_map = get_auxiliary_map();
/* Fill this map with the event header and all parameters we have to catch. */
aux_map_store_event_header(PPME_SYSCALL_OPEN_X, aux_map);
/* 1° Parameter: ret (type: PT_FD) */
aux_map_store_s64_param(ret, aux_map);
/* 2° Parameter: name (type: PT_FSPATH) */
unsigned long name = get_argument(regs, 0);
aux_map_store_charbuf_param(name, aux_map);
/* 3° Parameter: flags (type: PT_FLAGS32) */
unsigned long flags = get_argument(regs, 1);
aux_map_store_u32_param(open_flags_to_scap(sys_arg), aux_map);
/* 4° Parameter: mode (type: PT_UINT32) */
unsigned long mode = get_argument(regs, 2);
aux_map_store_u32_param(open_modes_to_scap(flags, mode), aux_map);
/* 5° Parameter: dev (type: PT_UINT32) */
unsigned long dev = extract_dev_from_fd(ret);
aux_map_store_u32_param(dev, aux_map);
/* Copy the content of the map into the ring buffer and push it to user-space. */
copy_aux_map_into_ringbuf(aux_map);
return 0;
} Obviously, the underlying technologies will be different, but if we are good enough, we could try to share the code between probes using only different helper functions that wrap different technologies! However, before doing this, we need CO-RE, libbpf, and a great refactor of our code: the state-of-the-art of our actual probe doesn't allow us to dream solutions like this. Anyway, I see this as a second step to avoid increasing the developer effort, as I said in the previous comment. |
Future thoughts:
If we can get the performance increase, and a migration path laid out I think it could be possible to move to a new probe all together. Although I am wondering what the other maintainers have to say about this. Approving for now. Happy to help test, develop, debug on my end. Great work. /approve |
Hi @kris-nova thank you very much for your support! I will try to address here your points:
If we are able to implement also just a subset of them, we can have a reliable sample for our tests. |
/ok-to-test |
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 can just say that this is very cool and very well presented. Congrats. I really like this proposal. So, it's time to approve it for me. 😺
/approve
LGTM label has been added. Git tree hash: 9bcb76d70d75173ce778802f0b3ff4111d85b6c8
|
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.
/approve
As already stated, huge +1 from me! Thanks for this great work!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, kris-nova, leogr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Andrea Terzolo andrea.terzolo@polito.it
What type of PR is this?
/kind design
Any specific area of the project related to this PR?
/area proposals
What this PR does / why we need it:
Hi all 🖖 With this PR I want to propose a possible design for a new BPF probe that exploits all modern tracing technologies to improve performance and usability. I spent some time dreaming about this solution, and now it finally seems within reach! Please don't forget that this is just a proposal to put together possible ideas so, don't be shy, if you have suggestions, don't hesitate to share your thoughts!
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?: