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
[DECISION/VOTE]: Clarify acceptability of a BPF probe-centric approach for custom driver management #1527
Comments
+1 I'm in favor, as I wasn't particularly convinced by hiding driver management away during the last major SC refactor. Flexibility will allow for faster innovation. While I envision a highly "intelligent" driver in the future, I also believe that end users should have some degree of control. For example, the |
@Molter73 since you are out I assume +1 for you as well since you opened the proposal in the first place. |
Hmm but what is it that we're actually discussing here?
For (3), does the unrelated stuff involve scap events in any way (existing schema or extensions) or does it simply use the libs as a convenient loader? ... or something else entirely? |
It's close the second point, except the discussion is not about adding anything In the PR referenced here, we're planning to make probe management more |
My personal opinion is that if the loader changes are needed only for a forked version of libs, they should live in the fork if possible. Please note that this does not mean I'm against eventually upstreaming your changes, but I'm simply not clear on what we accomplish by merging just the loader bits. You still have a whole fork to maintain, right? Are the loader bits more problematic for whatever reason? Do you want to use upstream falco (libs) with a custom probe? |
Nope, we were actually half way through dropping the fork in favour of the
Yes, the loaded (for both bpf & modern_probe) is very opinionated about how the |
@gnosek I actually understand this completely. Another way to look at it is that The Falco Project officially supports and benefits from adopters who build tools using libs. From my perspective, this makes such requests first-class citizens as well. And there will be a commitment to iterate and polish everything. With this in mind, does the justification seem improved from your perspective? |
Hmm not sure I understand how that's possible. You do need a custom probe (that's the entire issue) so you do have to maintain it somewhere, right? I'm genuinely trying to understand.
I feel that it has to be opinionated, or at the very least, the opinion has to live somewhere (we don't want to yolo attach all tracepoints we find). With a custom probe, the default loader has no idea what to do with the extra sections and I have a feeling they'd always be a second class citizen (the default loader wouldn't know when to attach a tracepoint).
Yup, this is a very good point, though I still feel that these custom probes should either get upstreamed eventually or just keep on living in their fork if they're not headed upstream. Still, I can imagine a better loader would reduce the initial friction of playing with a new probe so it might be worth a try. Looking through #1375, it seems like the vast majority of the patch is making the BPF program array dynamically-sized. Given the extent of changes required, I think I'd rather introduce a separate bpf_attached_prog_list for custom programs (btw, I have opinions about the name :P). For v1, we can unconditionally load all custom programs in load_single_prog and append their info to the new array (like in your patch), for v2 I'd rather see unused programs unloaded (or ideally not even loaded in the first place, avoiding potential verifier issues) but that's a general issue, not with your patch. Again for v1, enforce_sc_set can unconditionally attach/detach the programs based on handle->capturing, but I'd much rather see a callback there (defaulting to the handle->capturing logic so e.g. upstream falco doesn't need to care). In an ideal world, we'd have a way to control the custom tracepoints with some sort of callback (e.g. via ->configure) but I understand nobody needs this right now so let's ignore it ;) In scap_bpf_close, we detach and clean up all the custom programs from the extra table. (I had other, way more complex ideas for this but I believe this is a decent balance between giving custom probes the flexibility and minimizing impact on the built in tracepoints). |
No worries, it's fine :) In our case the custom bpf probe lives outside the |
Quick note: @gnosek I like the progression you described here #1527 (comment) very much and appreciate the technical details as well 😉 I also think that down the road (assuming the custom probe you maintain @erthalion is successful) it would be nice to consider upstreaming the probe. So is #1527 (comment) an ok, we have a path forward? |
I really appreciated the ongoing discussion and recognized some potential benefits, though I don't have strong opinions yet. I think clarity on the proposal's long-term implications, especially regarding maintenance costs and overall project direction, has been overlooked a bit, IMO. After having gone through all the comments, I realized I have more questions than answers 😅 I've tried to collect them and report them below to help reach clarity. Scope:
Maintenance Costs:
Drivers function parity:
Event schema compatibility:
Other ideas/proposals (eg LSM):
I really believe focusing on defining the proposal in detail before proceeding to a vote is crucial. In particular, I think goals, non-goals, and full extent must be specified before deciding. 🙏 |
Thanks for the discussion, folks. I'll try to follow up step-by-step.
@gnosek @incertum Just to make sure I got it right:
Does it mean the high level plan here is essentially what we are proposing,
@leogr I think it's important to distinguish those parts of the proposal that
I don't see any implications for the whole Falco project.
Don't think there would be any special impact, aside of regular PR review
As far as I see, the only alternative so far is do not have this feature at
This one depends on the implementation. The way we see it, the only maintenance
Depending on the implementation the scope could be different, but if everything
Again, heavily implementation dependent, but we don't see any significant
It's a very long-term vision, probably there could be different "flavours" of
What kind of disparities do you have in mind here?
Yep.
No, unless it fits into the future community development roadmap.
This proposal is orthogonal to those discussions, but could enhance any
Well, that's the whole topic of the proposal -- the best strategy we see is to |
Thank you @erthalion ! A few more comments point-by-point 👇
I'm unsure about this, and I would like the proposal to be clear in this regard. My goal just is to ensure everyone is on the same page. For instance, the implementation of this proposal might require some modification to the Driver API. The driver API is versioned, and the versioning allows to check the compatibility between a given version of Falco and a given version of a driver. Thanks to that, a given release of Falco is compatible with more driver releases.
I actually meant if this would be an item for the Falco project roadmap or not. If not, this should be kind of best effort, which is ok, but will have very low priority compared to Falco items (for example, the two months before the release we are usually at full capacity on Falco items).
It would be desirable to define a precise strategy and understand if this may affect the actual testing process since driver testing has been a huge effort historically (especially because of the verifier and testing across different versions of kernels, clang, etc..). Likely, the proposal need to define the implementation strategy, otherwise it would be hard to understand implications.
Good to know! 👍
No one atm. Just wondering. Generally speaking, if a user-facing feature can be only implemented and not in the kmod, this will create a disparity.
Ok, this is crystal clear now 👍 PS |
This proposal for now is about scap changes only to load a custom probe. Therefore, I don't see this impacting our drivers testing.
Since this is for libs only and not Falco for now, I also don't see a problem here. In fact we already have driver disparities in libs today. For instance we support some archs only for some drivers. Also I don't recall any such discussions for expanding archs support, so I am still a bit puzzled why we now shift criteria.
re the question for me @erthalion: I don't see why this would pose a problem or big impact for libs. Rather the opposite, it will allow for more custom libs adoption. So yes "make the loader flexible enough to load custom programs from the probe" 👍 @leogr would propose to keep this discussion more focused on supporting "loading a custom probe" and having "an agreement that we will allow the bpf loader to be flexible to do that". I think it will make libs much more flexible and drive libs adoption which is excellent for the overall project health. In addition, three engineers who already commit to maintaining it is a privilege in open source. Everything else I would defer to a formal proposal "overall driver and event filtering modernization" where we design the future of all things LSM hooks, kernel side filtering, event schemas and event handling etc. If no one else wants to lead starting such a proposal I can do it. |
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
@erthalion comment:
#1376 (comment)
@Stringy comment:
#1376 (comment)
@falcosecurity/libs-maintainers, please share your stance on this proposal. +1 indicates your support for enhancing custom driver management for libs adopters. The possible use case for Falco is not yet relevant.
Remember that libs serves as the foundation not only for Falco but also for other tools built upon it. Libs only adopters are crucial to the success of The Falco Project. Supporting and assisting libs only adopters in overcoming challenges that ultimately align with Falco's best interests benefits the entire community.
The text was updated successfully, but these errors were encountered: