-
Notifications
You must be signed in to change notification settings - Fork 182
update(libsinsp)!: merge all the opening methods into just one #540
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
Changes from all commits
08c4527
eaf7024
10f9049
da8ad0a
cfa33ae
74c9985
95cb451
603ef4f
ab19747
4a86b1f
eadf529
1c3e2d0
8c77c30
3e2a9a4
26dfd12
2c500f2
23037f3
04811fa
ac63424
1ee2193
ea60817
fdade85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| include_directories(${LIBSCAP_INCLUDE_DIRS} ../noop) | ||
| add_library(scap_engine_bpf scap_bpf.c bpf_public.c) | ||
| add_library(scap_engine_bpf scap_bpf.c) | ||
| if(NOT MINIMAL_BUILD) | ||
| target_link_libraries(scap_engine_bpf scap_event_schema scap_engine_util elf) | ||
| endif() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,6 @@ limitations under the License. | |
| #include <dirent.h> | ||
|
|
||
| #include "bpf.h" | ||
| #include "bpf_public.h" | ||
| #include "engine_handle.h" | ||
| #include "scap.h" | ||
| #include "scap-int.h" | ||
|
|
@@ -86,45 +85,9 @@ struct bpf_map_data { | |
| struct bpf_map_def def; | ||
| }; | ||
|
|
||
| static const char* resolve_bpf_probe(const char *bpf_probe, char *buf) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We receive the bpf path directly from the client, so we don't need anymore this :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment just below describes why we haven't done that in the first place :) Your change forces every consumer to do BPF probe discovery on their own. On the other hand, it might actually be good anyway ;) BTW, does this let us drop the env var
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this probably was born as a last attempt to open something, the problem is that some consumers like Falco use this workaround as the only way to open the probe and this is really strange IMHO 🤔 On the other side with this new approach is the consumer that decides the engine and which params it must have so we cannot rely anymore on this kind of approach :/
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, this is the reason I added the The method isn't used yet (in the submitted/merged PRs) but the end result would be: and the rest of libscap should be completely engine-agnostic (modulo the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uhm I understood, that seems a smart approach but since we are changing the sinsp interface we can solve the problem at the root. This seems the right time to perform this change since we are quite far from the libs tag and the consumers can have the right time to change the interface 🤔
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I'm not touching sinsp (my PRs are big enough as is) but if you're willing to open that can of worms, go ahead :) But my request if you do change the sinsp API would be to submit a PR to Falco at a similar time, so that consumers have at least something to base their changes on.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahahah yeah for sure, if we agree on that I can open a PR in Falco even before this one is merged :) |
||
| static bool match(scap_open_args* oargs) | ||
| { | ||
| // | ||
| // While in theory we could always rely on the scap caller to properly | ||
| // set a BPF probe from the environment variable, it's in practice easier | ||
| // to do one more check here in scap so we don't have to repeat the logic | ||
| // in all the possible users of the libraries | ||
| // | ||
| if(!bpf_probe) | ||
| { | ||
| bpf_probe = scap_get_bpf_probe_from_env(); | ||
| } | ||
|
|
||
| if(!bpf_probe) | ||
| { | ||
| return NULL; | ||
| } | ||
|
|
||
| if(strlen(bpf_probe) != 0) | ||
| { | ||
| strlcpy(buf, bpf_probe, SCAP_MAX_PATH_SIZE); | ||
| return buf; | ||
| } | ||
|
|
||
| const char *home = getenv("HOME"); | ||
| if(!home) | ||
| { | ||
| return NULL; | ||
| } | ||
|
|
||
| snprintf(buf, SCAP_MAX_PATH_SIZE, "%s/%s", home, SCAP_PROBE_BPF_FILEPATH); | ||
| return buf; | ||
| } | ||
|
|
||
| static bool match(scap_open_args* open_args) | ||
| { | ||
| char bpf_probe_buf[SCAP_MAX_PATH_SIZE]; | ||
|
|
||
| return !open_args->udig && resolve_bpf_probe(open_args->bpf_probe, bpf_probe_buf); | ||
| return strncmp(oargs->engine_name, BPF_ENGINE, BPF_ENGINE_LEN) == 0; | ||
| } | ||
|
|
||
| static struct bpf_engine* alloc_handle(scap_t* main_handle, char* lasterr_ptr) | ||
|
|
@@ -1757,43 +1720,43 @@ static int32_t configure(struct scap_engine_handle engine, enum scap_setting set | |
| return scap_bpf_set_statsd_port(engine, arg1); | ||
| default: | ||
| { | ||
| char msg[256]; | ||
| char msg[SCAP_LASTERR_SIZE]; | ||
| snprintf(msg, sizeof(msg), "Unsupported setting %d (args %lu, %lu)", setting, arg1, arg2); | ||
| return unsupported_config(engine, msg); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static int32_t init(scap_t* handle, scap_open_args *open_args) | ||
| static int32_t init(scap_t* handle, scap_open_args *oargs) | ||
| { | ||
| int32_t rc; | ||
| char bpf_probe_buf[SCAP_MAX_PATH_SIZE]; | ||
| const char* bpf_probe; | ||
| char buf[SCAP_LASTERR_SIZE]; | ||
| int32_t rc = 0; | ||
| char bpf_probe_buf[SCAP_MAX_PATH_SIZE] = {0}; | ||
| char error[SCAP_LASTERR_SIZE] = {0}; | ||
| struct scap_engine_handle engine = handle->m_engine; | ||
| struct scap_bpf_engine_params *params = oargs->engine_params; | ||
| strlcpy(bpf_probe_buf, params->bpf_probe, SCAP_MAX_PATH_SIZE); | ||
|
|
||
| bpf_probe = resolve_bpf_probe(open_args->bpf_probe, bpf_probe_buf); | ||
| // | ||
| // Find out how many devices we have to open, which equals to the number of CPUs | ||
| // | ||
| ssize_t num_cpus = sysconf(_SC_NPROCESSORS_ONLN); | ||
| if(num_cpus == -1) | ||
| { | ||
| snprintf(engine.m_handle->m_lasterr, SCAP_LASTERR_SIZE, "_SC_NPROCESSORS_ONLN: %s", scap_strerror_r(buf, errno)); | ||
| snprintf(engine.m_handle->m_lasterr, SCAP_LASTERR_SIZE, "_SC_NPROCESSORS_ONLN: %s", scap_strerror_r(error, errno)); | ||
| return SCAP_FAILURE; | ||
| } | ||
|
|
||
| engine.m_handle->m_ncpus = num_cpus; | ||
|
|
||
| fill_syscalls_of_interest(&open_args->ppm_sc_of_interest, &engine.m_handle->m_syscalls_of_interest); | ||
| fill_syscalls_of_interest(&oargs->ppm_sc_of_interest, &engine.m_handle->m_syscalls_of_interest); | ||
|
|
||
| rc = devset_init(&engine.m_handle->m_dev_set, num_cpus, engine.m_handle->m_lasterr); | ||
| if(rc != SCAP_SUCCESS) | ||
| { | ||
| return rc; | ||
| } | ||
|
|
||
| rc = scap_bpf_load(engine.m_handle, bpf_probe, &handle->m_api_version, &handle->m_schema_version); | ||
| rc = scap_bpf_load(engine.m_handle, bpf_probe_buf, &handle->m_api_version, &handle->m_schema_version); | ||
| if(rc != SCAP_SUCCESS) | ||
| { | ||
| return rc; | ||
|
|
@@ -1855,7 +1818,7 @@ const struct scap_vtable scap_bpf_engine = { | |
|
|
||
| #else // MINIMAL_BUILD | ||
|
|
||
| static int32_t init(scap_t* handle, scap_open_args *open_args) | ||
| static int32_t init(scap_t* handle, scap_open_args *oargs) | ||
| { | ||
| strlcpy(handle->m_lasterr, "The eBPF probe driver is not supported when using a minimal build", SCAP_LASTERR_SIZE); | ||
| return SCAP_NOT_SUPPORTED; | ||
|
|
@@ -1885,4 +1848,3 @@ const struct scap_vtable scap_bpf_engine = { | |
| .getpid_global = noop_getpid_global, | ||
| }; | ||
| #endif // MINIMAL_BUILD | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,10 +45,11 @@ static SCAP_HANDLE_T *gvisor_alloc_handle(scap_t* main_handle, char *lasterr_ptr | |
| return new scap_gvisor::engine(lasterr_ptr); | ||
| } | ||
|
|
||
| static int32_t gvisor_init(scap_t* main_handle, scap_open_args* open_args) | ||
| static int32_t gvisor_init(scap_t* main_handle, scap_open_args* oargs) | ||
| { | ||
| scap_gvisor::engine *gv = main_handle->m_engine.m_handle; | ||
| return gv->init(open_args->gvisor_config_path, open_args->gvisor_root_path); | ||
| struct scap_gvisor_engine_params *params = (struct scap_gvisor_engine_params *)oargs->engine_params; | ||
| return gv->init(params->gvisor_config_path, params->gvisor_root_path); | ||
| } | ||
|
|
||
| static void gvisor_free_handle(struct scap_engine_handle engine) | ||
|
|
@@ -76,9 +77,9 @@ static int32_t gvisor_next(struct scap_engine_handle engine, scap_evt **pevent, | |
| return engine.m_handle->next(pevent, pcpuid); | ||
| } | ||
|
|
||
| static bool gvisor_match(scap_open_args* open_args) | ||
| static bool gvisor_match(scap_open_args* oargs) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need such a match API when we can just |
||
| { | ||
| return open_args->gvisor_config_path != NULL; | ||
| return strncmp(oargs->engine_name, GVISOR_ENGINE, GVISOR_ENGINE_LEN) == 0; | ||
| } | ||
|
|
||
| static int32_t gvisor_configure(struct scap_engine_handle engine, enum scap_setting setting, unsigned long arg1, unsigned long arg2) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /* | ||
| Copyright (C) 2022 The Falco Authors. | ||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #define GVISOR_ENGINE "gvisor" | ||
| #define GVISOR_ENGINE_LEN 7 | ||
|
|
||
| #ifdef __cplusplus | ||
| extern "C" | ||
| { | ||
| #endif | ||
|
|
||
| struct scap_gvisor_engine_params | ||
| { | ||
| const char* gvisor_root_path; ///< When using gvisor, the root path used by runsc commands | ||
| const char* gvisor_config_path; ///< When using gvisor, the path to the configuration file | ||
| }; | ||
|
|
||
| #ifdef __cplusplus | ||
| }; | ||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| /* | ||
| Copyright (C) 2022 The Falco Authors. | ||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <stdint.h> | ||
|
|
||
| #define KMOD_ENGINE "kmod" | ||
| #define KMOD_ENGINE_LEN 5 | ||
|
|
||
| #ifdef __cplusplus | ||
| extern "C" | ||
| { | ||
| #endif | ||
|
|
||
| struct scap_kmod_engine_params | ||
| { | ||
| uint64_t single_buffer_dim; ///< dim of a single shared buffer. Usually, we have one buffer for every online CPU. | ||
| }; | ||
|
|
||
| #ifdef __cplusplus | ||
| }; | ||
| #endif |
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.
Left comments in falcosecurity/falco#2164.
Therefore only posting a small cross-reference here: Trusting end user input typically is terrifying especially for such an important parameter. How about brainstorming on what a sanitization logic could look like? Could even include correcting the value to the nearest mathematically correct value? CC @gnosek
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.
yeah, absolutely we need to check that these values are correct. As I said in Falco if we use the number of pages we probably need to check only that is a positive value and that it is a power of 2. The sanitization logic is a good idea, not sure if defaulting it to the nearest value or using the default value since the provided one is not correct 🤔
This PR just introduces this field in the
scap-openargs, I will allow the driver to use it in another one, so probably i will put the checks in that PR, BTW having a clear idea on how to proceed from the beginning seems reasonable to me!P.S @gnosek @FedeDP do you remember if also for the kmod we need a buffer dimension that is a power of 2?
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.
Basically the only requirement I'm aware of is that it needs to be mmapped back-to-back twice without gaps, so it must be a multiple of page size (usually 4K, up to 16 or 64? on some arches). Same goes for udig, BTW.
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.
Good to know, thanks @gnosek! @Andreagit97 could see how correcting to nearest correct value can indeed terribly backfire. Suggested hard-coding an array of 10-12 values to choose from in the other PR. That way nothing should go wrong in the sanitization check and it will be simple.
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.
Uhm I do not completely agree with a fixed array because otherwise every time someone wants to try a new dimension we have to open a new pull request, and we lose in flexibility, maybe upper/lower bound checks are enough 🤔 BTW I will move this conversation on the Falco PR, the implementation/validation of variable size buffers will take place in another PR here we have introduced only the argument and since this PR it's quite blocking I wouldn't stop all the flow for this reason, WDYT?