#general - Thank you

Channels

a
Alexander
1 year ago
Thank you
Stefano Bonicatti
1 year ago
So heads up, updating the toolchain might be slightly more involved. The CI (and the common build) always uses a previously built and released version of the toolchain. Doing this release requires several steps. Currently the repository is more up to date vs the version of the toolchain that's released, and while I wanted to do a new release with the toolchain using LLVM 11, I realized that we have to correct the usage of the kernel headers. They were set to version 4.9 back then due to some BPF dependencies but this is partially incorrect because we support CentOS 6. What we should do toolchain wise is actually to compile against CentOS 6 kernel version of headers, then osquery has to define the newer kernels defines and at runtime detect if some feature is available or not.
But I also need to actually explore this... because there might effectively be issues with the process of preparing the toolchain itself when setting such old headers. Nonetheless, I would say that the PR currently should not require an update of the toolchain, but provide the define if missing; it should also work correctly on kernel versions that do not possess the latest features.
a
Alexander
1 year ago
Got it, we can just define missing constants in our source, but that requires some "ugly" decisions, like should we define only constants that are missing in 4.7, or all used constants. I though updating headers in toolchain would be better than trying to make questionable constants in osquery itself, that would become redundant in future
Stefano Bonicatti
1 year ago
I see what you mean; my interest is mainly trying not to block your PR and at the same time try to adhere to what I was saying earlier. I haven't checked specifically, but lets say we're not updating the toolchain right now, which constants have to be defined for the code to compile? Updating the toolchain really needs some tests to see how it works with older distros, that's something we really want to have.
a
Alexander
1 year ago
I should recheck, as that patch was done a year before (we used it internally for a year). Most probably the missing constants are: SECCOMP_RET_KILL_PROCESS and SECCOMP_RET_LOG, both introduced in 4.14 https://github.com/torvalds/linux/commit/4d3b0b05aae9ee9ce0970dc4cc0fb3fad5e85945
Stefano Bonicatti
1 year ago
I see, then that might be enough for that issue. Thank you for understanding!
a
Alexander
1 year ago
SECCOMP_RET_KILL_THREAD also need to be checked, accouding to
```
man seccomp
```
it replaced SECCOMP_RET_KILL in 4.14
Stefano Bonicatti
1 year ago
Maybe that's a rename only though.
though in the diff you linked it seems they are both still present; seems what they have done is to add a
```
SECCOMP_RET_KILL_PROCESS
```
and so instead of referring to the more generic
```
SECCOMP_RET_KILL
```
they use
```
SECCOMP_RET_KILL_THREAD
```
to differentiate, but they have the same value.

Alexander

1 year ago

Yes, but thatSECCOMP_RET_KILL_THREAD

not in 4.13 source, so there must be some other 4.14 patch that introduced new constant.

Stefano Bonicatti
1 year ago
oh I see, I was assuming they were both present and only manual changed. Nonetheless it seems to me that
```
SECCOMP_RET_KILL
```
retained its original value, I'm looking at 4.9 code
so the value didn't change anyway
But yeah the define might have to be added to the osquery source code. Sorry I was thinking you were referring to some value change
a
Alexander
1 year ago
Ok. @Anton Zhabolenko, I think you can read our thread.
BTW, @User can you advise where should we put those ifndef-s in osquery tree? We can just put it here - close to
```
#include <linux/seccomp.h>
```
or make some fancy wrapper to seccomp.h, but i don't see appropriate place for wrappers. And should it be in-tree, or it better to be some logic around kernel version in CMakeFile?
Stefano Bonicatti
1 year ago
That's a good question. For simplicity I would just put them inline for now, under the
```
linux/seccomp.h
```
header. That is the only usage now and I suspect for a while too. We'll handle moving them somewhere else if needed. But definitely if you can, put them under
```
ifndef
```
so that we can play nice with other kind of builds.

Join thread in Slack

View count: 1