-
Notifications
You must be signed in to change notification settings - Fork 886
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
make the falco driver loader downloads first, then fallback to compilation #1599
Conversation
…IP_DRIVER_LOADER When we started to implemented 20200506-artifacts-scope-part-2 proposal , among a million other things, we renamed `SKIP_MODULE_LOAD` to `SKIP_DRIVER_LOADER`. We reatained compatibility with `SKIP_MODULE_LOAD` for a bunch of releases. Now, after 9 months have passed I think it's time to completely deprecate it. Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
…mpile it on-the-fly Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
…o compile one Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Ready for review even if WIP |
/milestone 0.29.0 |
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.
Overall, it seems very good to me 👏
I have just found an issue that should easily be fixable (see the comment below about the need of mounting debugfs
).
Other comments are just small thing, not really needed :)
scripts/falco-driver-loader
Outdated
echo "* Mounting debugfs" | ||
|
||
if [ ! -d /sys/kernel/debug/tracing ]; then | ||
mount -t debugfs nodev /sys/kernel/debug | ||
fi |
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.
Under some circumstances, mounting debugfs
is required to run Falco with the eBPF probe, but this piece seems to be lost in the refactoring.
The driver-loader/integration
test is failing for the missing mount (logs here).
Indeed, without that, Falco fails to load the probe and exits immediately:
$ FALCO_BPF_PROBE="" falco
Tue Apr 6 16:33:51 2021: Falco version 0.27.0-86+787eebb (driver version 5c0b863ddade7a45568c0ac97d037422c9efb750)
Tue Apr 6 16:33:51 2021: Falco initialized with configuration file /etc/falco/falco.yaml
Tue Apr 6 16:33:51 2021: Loading rules from file /etc/falco/falco_rules.yaml:
Tue Apr 6 16:33:51 2021: Loading rules from file /etc/falco/falco_rules.local.yaml:
Tue Apr 6 16:33:52 2021: Loading rules from file /etc/falco/k8s_audit_rules.yaml:
Tue Apr 6 16:33:52 2021: Unable to load the driver.
Tue Apr 6 16:33:52 2021: Runtime error: failed to open event raw_syscalls/sys_enter. Exiting.
Instead, after running mount -t debugfs nodev /sys/kernel/debug
then Falco works as expected.
So, my advice here is just to restore those lines.
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.
This is needed in systems where raw tracepoints are not available.
Agreed with @leogr that this needs to be reverted.
See:
https://github.com/falcosecurity/libs/blob/master/userspace/libscap/scap_bpf.c#L474-L485
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 remember we discussed this privately. Anyways, I'm gonna put it back ASAP.
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.
Thanks Leo, I agree we need to do this but I would prefer to do it in a second moment if you agree
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.
Ideally, the correct place for this mount would be in Falco binary when the FALCO_BPF_PROBE
env variable is present.
I'll revert it back for now, anyways.
scripts/falco-driver-loader
Outdated
echo "* Mounting debugfs" | ||
|
||
if [ ! -d /sys/kernel/debug/tracing ]; then | ||
mount -t debugfs nodev /sys/kernel/debug | ||
fi |
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.
This is needed in systems where raw tracepoints are not available.
Agreed with @leogr that this needs to be reverted.
See:
https://github.com/falcosecurity/libs/blob/master/userspace/libscap/scap_bpf.c#L474-L485
…iver-loader Co-authored-by: Lorenzo Fontana <lo@linux.com> Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
This is needed in systems where raw tracepoints are not available. Anyways, since this is needed when the inspector open (and actually loads) the eBPF probe, ideally the mount should not be done by this script but rather from Falco, or from Falco libs. Otherwise, users building the eBPF probe theirseleves and not using this script (and having a kernel without raw tracepoints) may need to mount this fs theirselves. Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
/milestone 0.28.0 |
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.
LGTM! 👍
LGTM label has been added. Git tree hash: f6ffb1c68c7f69ec073524101d2807b1b341aba3
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fntlnz, 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 |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
ALL (since we don't have a scripts area).
What this PR does / why we need it:
This PR inverts the logic of the
falco-driver-loader
script.The new logic (expand 👇 ) is:
kernel module case (default one)
falco.ko
) if present/lib/modules/$(uname -r)
- exit in case operation succeeds (notice that the driver version is not checked)falco_{distro}_{kernel_release}_{kernel_version}.ko
- in${HOME}/.falco/
- insert it if present (notice that the driver version is not checked)eBPF probe case
falco_{distro}_{kernel_release}_{kernel_version}.o
- in${HOME}/.falco/
- go to 4 (notice that the driver version is not checked)${HOME}/.falco/falco_{distro}_{kernel_release}_{kernel_version}.o
to${HOME/.falco/falco-bpf.o
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The reason for this PR is to reduce the number of cases in which the Falco users need to wait for a Falco container to build a driver on the fly.
This will also speed up the boot of containerized Falco (assuming a prebuilt exists for the host machine, we now have ~4K prebuilt Falco drivers at https://download.falco.org).
The only cases in which https://download.falco.org will not be hit by a request will be:
${HOME}/.falco/falco_{distro}_{kernel_release}_{kernel_version}.o
${HOME}/.falco/falco_{distro}_{kernel_release}_{kernel_version}.ko
In this way, the Falco community could also have more real statistics about the Falco usage and usage patterns by looking at the logs of https://download.falco.org.
Notes that this PR also moves out from the
load_bpf_probe
function the target detection logic in its place (get_target_id
) forminikube
andCOS
.Does this PR introduce a user-facing change?: