From 2e76d0d679920984ee2d349d0659ceb48941d0e9 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Thu, 28 Sep 2023 12:02:56 +0200 Subject: [PATCH] cleanup(libscap): add some comments and minor cleanups to the CPU logic Signed-off-by: Andrea Terzolo --- userspace/libpman/src/ringbuffer.c | 10 ++++++++++ userspace/libscap/engine/bpf/scap_bpf.c | 22 ++++++++++++---------- userspace/libscap/engine/kmod/scap_kmod.c | 20 +++++++++++--------- 3 files changed, 33 insertions(+), 19 deletions(-) diff --git a/userspace/libpman/src/ringbuffer.c b/userspace/libpman/src/ringbuffer.c index 5a1a00410e..b4a41ba1f5 100644 --- a/userspace/libpman/src/ringbuffer.c +++ b/userspace/libpman/src/ringbuffer.c @@ -201,6 +201,16 @@ int pman_finalize_ringbuf_array_after_loading() continue; } + if(ringbuf_id >= g_state.n_required_buffers) + { + /* If we arrive here it means that we have too many CPUs for our allocated ring buffers + * so probably we faced a CPU hotplug. + */ + snprintf(error_message, MAX_ERROR_MESSAGE_LEN, "the actual system configuration requires more than '%d' ring buffers", g_state.n_required_buffers); + pman_print_error((const char *)error_message); + goto clean_percpu_ring_buffers; + } + if(bpf_map_update_elem(ringubuf_array_fd, &i, &ringbufs_fds[ringbuf_id], BPF_ANY)) { snprintf(error_message, MAX_ERROR_MESSAGE_LEN, "failed to add the ringbuf map for CPU '%d' to ringbuf '%d'", i, ringbuf_id); diff --git a/userspace/libscap/engine/bpf/scap_bpf.c b/userspace/libscap/engine/bpf/scap_bpf.c index 39a5783d95..3c68a5a923 100644 --- a/userspace/libscap/engine/bpf/scap_bpf.c +++ b/userspace/libscap/engine/bpf/scap_bpf.c @@ -1521,6 +1521,8 @@ int32_t scap_bpf_load( // struct scap_device_set *devset = &handle->m_dev_set; uint32_t online_idx = 0; + // devset->m_ndevs = online CPUs in the system. + // handle->m_ncpus = available CPUs in the system. for(uint32_t cpu_idx = 0; online_idx < devset->m_ndevs && cpu_idx < handle->m_ncpus; ++cpu_idx) { struct perf_event_attr attr = { @@ -1528,8 +1530,8 @@ int32_t scap_bpf_load( .type = PERF_TYPE_SOFTWARE, .config = PERF_COUNT_SW_BPF_OUTPUT, }; - int pmu_fd; - int ret; + int pmu_fd = 0; + int ret = 0; /* We suppose that CPU 0 is always online, so we only check for cpu_idx > 0 */ if(cpu_idx > 0) @@ -1553,7 +1555,7 @@ int32_t scap_bpf_load( online = 1; } // If we can't access the cpu, count it as offline. - // Some VMs or hypertreading systems export an high number of configured CPUs, + // Some VMs or hyperthreading systems export an high number of configured CPUs, // even if they are not existing. See https://github.com/falcosecurity/falco/issues/2843 for example. // Skip them. } @@ -1606,21 +1608,21 @@ int32_t scap_bpf_load( online_idx++; } - // Check that we parsed all onlince CPUs + // Check that we parsed all online CPUs if(online_idx != devset->m_ndevs) { - return scap_errprintf(handle->m_lasterr, 0, "processors online: %d, expected: %d", online_idx, devset->m_ndevs); + return scap_errprintf(handle->m_lasterr, 0, "mismatch, processors online after the 'for' loop: %d, '_SC_NPROCESSORS_ONLN' before the 'for' loop: %d", online_idx, devset->m_ndevs); } // Check that no CPUs were hotplugged during the for loop uint32_t final_ndevs = sysconf(_SC_NPROCESSORS_ONLN); if(final_ndevs == -1) { - return scap_errprintf(handle->m_lasterr, errno, "_SC_NPROCESSORS_ONLN"); + return scap_errprintf(handle->m_lasterr, errno, "cannot obtain the number of online CPUs from '_SC_NPROCESSORS_ONLN' to check against the previous value"); } - if (final_ndevs != online_idx) + if (online_idx != final_ndevs) { - return scap_errprintf(handle->m_lasterr, 0, "wrong number of online processors: %d, expected: %d", final_ndevs, online_idx); + return scap_errprintf(handle->m_lasterr, 0, "mismatch, processors online after the 'for' loop: %d, '_SC_NPROCESSORS_ONLN' after the 'for' loop: %d", online_idx, final_ndevs); } @@ -1949,7 +1951,7 @@ static int32_t init(scap_t* handle, scap_open_args *oargs) ssize_t num_cpus = sysconf(_SC_NPROCESSORS_CONF); if(num_cpus == -1) { - return scap_errprintf(engine.m_handle->m_lasterr, errno, "_SC_NPROCESSORS_CONF"); + return scap_errprintf(engine.m_handle->m_lasterr, errno, "cannot obtain the number of available CPUs from '_SC_NPROCESSORS_CONF'"); } engine.m_handle->m_ncpus = num_cpus; @@ -1957,7 +1959,7 @@ static int32_t init(scap_t* handle, scap_open_args *oargs) ssize_t num_devs = sysconf(_SC_NPROCESSORS_ONLN); if(num_devs == -1) { - return scap_errprintf(engine.m_handle->m_lasterr, errno, "_SC_NPROCESSORS_ONLN"); + return scap_errprintf(engine.m_handle->m_lasterr, errno, "cannot obtain the number of online CPUs from '_SC_NPROCESSORS_ONLN'"); } rc = devset_init(&engine.m_handle->m_dev_set, num_devs, engine.m_handle->m_lasterr); diff --git a/userspace/libscap/engine/kmod/scap_kmod.c b/userspace/libscap/engine/kmod/scap_kmod.c index aa4aa29691..47e05b14a3 100644 --- a/userspace/libscap/engine/kmod/scap_kmod.c +++ b/userspace/libscap/engine/kmod/scap_kmod.c @@ -336,7 +336,7 @@ int32_t scap_kmod_init(scap_t *handle, scap_open_args *oargs) ncpus = sysconf(_SC_NPROCESSORS_CONF); if(ncpus == -1) { - return scap_errprintf(handle->m_lasterr, errno, "_SC_NPROCESSORS_CONF"); + return scap_errprintf(handle->m_lasterr, errno, "cannot obtain the number of available CPUs from '_SC_NPROCESSORS_CONF'"); } // @@ -345,7 +345,7 @@ int32_t scap_kmod_init(scap_t *handle, scap_open_args *oargs) ndevs = sysconf(_SC_NPROCESSORS_ONLN); if(ndevs == -1) { - return scap_errprintf(handle->m_lasterr, errno, "_SC_NPROCESSORS_ONLN"); + return scap_errprintf(handle->m_lasterr, errno, "cannot obtain the number of online CPUs from '_SC_NPROCESSORS_ONLN'"); } rc = devset_init(&engine.m_handle->m_dev_set, ndevs, handle->m_lasterr); @@ -361,14 +361,16 @@ int32_t scap_kmod_init(scap_t *handle, scap_open_args *oargs) struct scap_device_set *devset = &engine.m_handle->m_dev_set; uint32_t online_idx = 0; - for(uint32_t all_scanned_devs = 0; online_idx < devset->m_ndevs && all_scanned_devs < ncpus; ++all_scanned_devs) + // devset->m_ndevs = online CPUs in the system. + // ncpus = available CPUs in the system. + for(uint32_t cpu_idx = 0; online_idx < devset->m_ndevs && cpu_idx < ncpus; ++cpu_idx) { struct scap_device *dev = &devset->m_devs[online_idx]; // // Open the device // - snprintf(filename, sizeof(filename), "%s/dev/" DRIVER_DEVICE_NAME "%d", scap_get_host_root(), all_scanned_devs); + snprintf(filename, sizeof(filename), "%s/dev/" DRIVER_DEVICE_NAME "%d", scap_get_host_root(), cpu_idx); if((dev->m_fd = open(filename, O_RDWR | O_SYNC)) < 0) { @@ -493,21 +495,21 @@ int32_t scap_kmod_init(scap_t *handle, scap_open_args *oargs) ++online_idx; } - // Check that we parsed all onlince CPUs + // Check that we parsed all online CPUs if(online_idx != devset->m_ndevs) { - return scap_errprintf(handle->m_lasterr, 0, "processors online: %d, expected: %d", online_idx, devset->m_ndevs); + return scap_errprintf(handle->m_lasterr, 0, "mismatch, processors online after the 'for' loop: %d, '_SC_NPROCESSORS_ONLN' before the 'for' loop: %d", online_idx, devset->m_ndevs); } // Check that no CPUs were hotplugged during the for loop uint32_t final_ndevs = sysconf(_SC_NPROCESSORS_ONLN); if(final_ndevs == -1) { - return scap_errprintf(handle->m_lasterr, errno, "_SC_NPROCESSORS_ONLN"); + return scap_errprintf(handle->m_lasterr, errno, "cannot obtain the number of online CPUs from '_SC_NPROCESSORS_ONLN' to check against the previous value"); } - if (final_ndevs != online_idx) + if (online_idx != final_ndevs) { - return scap_errprintf(handle->m_lasterr, 0, "wrong number of online processors: %d, expected: %d", final_ndevs, online_idx); + return scap_errprintf(handle->m_lasterr, 0, "mismatch, processors online after the 'for' loop: %d, '_SC_NPROCESSORS_ONLN' after the 'for' loop: %d", online_idx, final_ndevs); } /* Here we are covering the case in which some syscalls don't have an associated ppm_sc