tracee icon indicating copy to clipboard operation
tracee copied to clipboard

eBPF CO-RE improvements: remove non CO-RE source code

Open grantseltzer opened this issue 3 years ago • 9 comments
trafficstars

Continuing to maintain non-CORE bpf code adds more complexity to the bpf code, build system, and logistics for installing/running. With the addition of BTFHub support we should be able to remove support for non-CORE code. This would involve removing the IFDEF macros in bpf code, and build file recipes

grantseltzer avatar Apr 08 '22 14:04 grantseltzer

This seems like a logical step for the future of Tracee, however, as we already have such a support implemented, I think we should wait for a while with removing it, at least until BTF will become more common on existing kernels. I would say a year from now would be a better time to remove support for non -core bpf code

yanivagman avatar Apr 09 '22 05:04 yanivagman

This seems like a logical step for the future of Tracee, however, as we already have such a support implemented, I think we should wait for a while with removing it, at least until BTF will become more common on existing kernels. I would say a year from now would be a better time to remove support for non -core bpf code

We have a list of supported environments for Tracee and we can add support for any environment that does not have BTF enabled in BTFHUB. That, per se, is already something that enables us to drop non CO-RE code.

What is the supported environment you currently see not supported in BTFHUB ? Should we add support there ?

rafaeldtinoco avatar Apr 11 '22 13:04 rafaeldtinoco

As long as non-core environments are still common, I don't think BTFhub is enough. When you spot an environment for which you need to genereate a new BTF file, you can't immediately start using Tracee on that environment as it takes time to report this missing environment in BTFHUB and generate a BTF for it, not to mention that the already embedded BTFs we have in Tracee won't be enough, and the user will have to wait for the next version of Tracee to be out before he will be able to use it (assuming that he reported on the missing BTF to BTFHub, and that it was added in time before the next release).

There are few cases where this can happen today:

  1. LTS kernels without BTF support, where new patches appear from time to time
  2. Distros that are not currently in BTFHub - there are so many distros out there, and only a portion of it is available in BTFHub
  3. Custom compiled kernels

For this reason, I think we should keep this support for now

yanivagman avatar Apr 14 '22 07:04 yanivagman

As long as non-core environments are still common, I don't think BTFhub is enough.

I don't think they're common, that is the point. And we can't guarantee supportability in any existing environment IMHO. BTFhub was meant to add support to LTS distributions without BTF support that we currently TEST and SUPPORT (or the requirements would be to use a kernel that supports BTF).

When you spot an environment for which you need to generate a new BTF file, you can't immediately start using Tracee on that environment as it takes time to report this missing environment in BTFHUB and generate a BTF for it,

That is why I asked about such environments. Have you spotted any LTS environment that needs attention ? Development and Playground environments should stick with BTF enabled kernels.

The only LTS Linux version that is <= 5.1-rc4 (CONFIG_DEBUG_INFO_BTF) and >= 4.19 (our min kernel version support) is Debian 9 and 10, and we currently have it in BTFHUB, and RHEL8 with kernel 4.19, which has CONFIG_DEBUG_INFO_BTF backported.

not to mention that the already embedded BTFs we have in Tracee won't be enough, and the user will have to wait for the next version of Tracee to be out before he will be able to use it (assuming that he reported on the missing BTF to BTFHub, and that it was added in time before the next release).

So we're currently worrying about non CO-RE only for 4.19 <= X <= 5.1-rc4 and the only Linux distribution we should, for sure, support is Debian (already in BTFHUB). Let's say someone wants to use Debian 10 and Tracee, the only requirement would be for them to use one of these kernels:

/x86_64/4.19.0-18-cloud-amd64.btf.tar.xz
./x86_64/4.19.0-20-cloud-amd64.btf.tar.xz
./x86_64/4.19.0-18-amd64.btf.tar.xz
./x86_64/4.19.0-18-rt-amd64.btf.tar.xz
./x86_64/4.19.0-17-amd64.btf.tar.xz
./x86_64/4.19.0-20-amd64.btf.tar.xz
./x86_64/4.19.0-17-cloud-amd64.btf.tar.xz
./x86_64/4.19.0-20-rt-amd64.btf.tar.xz
./x86_64/4.19.0-17-rt-amd64.btf.tar.xz

which is a fair requirement.

There are few cases where this can happen today:

  1. LTS kernels without BTF support, where new patches appear from time to time

Users should stick with supported (by BTFHUB) kernels until we release next version, that has been discussed when BTFHUB was being created (and this situation would only occur for Debian, which is a distribution that does NOT have a commercial support plan).

  1. Distros that are not currently in BTFHub - there are so many distros out there, and only a portion of it is available in BTFHub

Yes but we're not supporting all of them. And the burden of maintaining non CO-RE code version for supporting something in between v4.19 and v5.1-rc4 is very big for generic cases.

  1. Custom compiled kernels

Custom compiled kernels should stick with BTF enabled kernels.

For this reason, I think we should keep this support for now

I'm happy to close this issue, but I disagree to adding burden on developing and maintaining a complex-enough code base, with a very small development team, because of feeling. I'd like to have specific cases discussed (based on premise of Linux distributions the project should support).

rafaeldtinoco avatar Apr 14 '22 10:04 rafaeldtinoco

My opinion is that without specific examples of kernels/users/customers that this would exclude support for, we can't justify keeping the overhead of maintaining the non-core code. As a compromise, I would be fine with keeping the non-core code in separate file that we would deprecate (and give proper warning for) while we add features to the core enabled code.

grantseltzer avatar Apr 18 '22 15:04 grantseltzer

We can use a separate file for non-core, as @grantseltzer suggested. All the CORE ifdefs should be either in the macros part (e.g. READ_KERN), or in the kernel version dependant section. We SHOULDN'T have such ifdefs in the bpf programs themselves (as was recently added with the addition of dirty_pipe event). Then, keeping these macros and kernel version specific helpers in a different file will be easier to maintain

yanivagman avatar Apr 23 '22 07:04 yanivagman

I would like to style the C code before major changes. I'm going to enforce styling no C files and headers @yanivagman and @grantseltzer. I haven't done that because it would cause major rebase needs in existing PRs and we had many after the 0.7 release (so I was holding this for awhile). Sounds good ? Style will be close to kernel source code styling and enforced on PRs like we do with gofmt.

rafaeldtinoco avatar Apr 25 '22 09:04 rafaeldtinoco

AND, ok @yanivagman I'm good if we're able to split CORE and non CO-RE code and make it better to read, maintain and... more elegant for general readers as well =o). We should deprecate it officially though, and make a roadmap to drop it as now we will have CO-RE testing daily and weekly and CO-RE support will improve a lot.

rafaeldtinoco avatar Apr 25 '22 09:04 rafaeldtinoco

Started doing this in https://github.com/aquasecurity/tracee/pull/1847 but after discussing with the team, this will be part of v0.9.0, and instead of separating the core/non-core we'll fully remove non-core code.

grantseltzer avatar Aug 02 '22 15:08 grantseltzer

This seems like a logical step for the future of Tracee, however, as we already have such a support implemented, I think we should wait for a while with removing it, at least until BTF will become more common on existing kernels. I would say a year from now would be a better time to remove support for non -core bpf code

The time has come... :-)

yanivagman avatar Apr 19 '23 18:04 yanivagman