diff --git a/driver/modern_bpf/helpers/base/maps_getters.h b/driver/modern_bpf/helpers/base/maps_getters.h index 6e4e0afc0..d9aa534c0 100644 --- a/driver/modern_bpf/helpers/base/maps_getters.h +++ b/driver/modern_bpf/helpers/base/maps_getters.h @@ -184,8 +184,51 @@ static __always_inline uint16_t maps__get_ppm_sc(uint16_t syscall_id) { /*=============================== AUXILIARY MAPS ===========================*/ static __always_inline struct auxiliary_map *maps__get_auxiliary_map() { - uint32_t cpu_id = (uint32_t)bpf_get_smp_processor_id(); - return (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_maps, &cpu_id); + /* Key by `pid_tgid` rather than CPU: BPF programs are preemptible on + * kernels >= 5.11, so a per-CPU scratch buffer can be clobbered by + * another program scheduled on the same CPU. A task only ever runs on + * one CPU at a time, so `pid_tgid` uniquely identifies an in-flight + * event build (stable across the tail-call chain). See + * falcosecurity/libs#2719. + */ + uint64_t pid_tgid = bpf_get_current_pid_tgid(); + struct auxiliary_map *auxmap = + (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_maps, &pid_tgid); + if(auxmap) { + return auxmap; + } + + /* First event for this task (or the entry was evicted from the LRU): we + * need to create the entry. `bpf_map_update_elem` requires a value to + * copy from; use the single-element init template (a 128 KB value cannot + * live on the BPF stack). The auxmap does not need to be zeroed (the + * header, payload_pos and lengths_pos are set explicitly by + * auxmap__preload_event_header, and param data is written before it is + * read), so the template contents are irrelevant. */ + uint32_t zero = 0; + struct auxiliary_map *init = + (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_map_init, &zero); + if(!init) { + return NULL; + } + if(bpf_map_update_elem(&auxiliary_maps, &pid_tgid, init, BPF_ANY)) { + return NULL; + } + return (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_maps, &pid_tgid); +} + +/** + * @brief Release the current task's auxiliary map entry. + * + * Called once an event has been submitted (best effort). Freeing the entry + * immediately keeps the `auxiliary_maps` LRU hash populated only with events + * that are currently being built (bounded by the CPU count), rather than one + * entry per task that has ever produced an event. Entries left behind by + * abandoned events (e.g. a failed tail call) are reclaimed by LRU eviction. + */ +static __always_inline void maps__release_auxiliary_map() { + uint64_t pid_tgid = bpf_get_current_pid_tgid(); + bpf_map_delete_elem(&auxiliary_maps, &pid_tgid); } /*=============================== AUXILIARY MAPS ===========================*/ diff --git a/driver/modern_bpf/helpers/store/auxmap_store_params.h b/driver/modern_bpf/helpers/store/auxmap_store_params.h index a74bc89b9..daaaa2061 100644 --- a/driver/modern_bpf/helpers/store/auxmap_store_params.h +++ b/driver/modern_bpf/helpers/store/auxmap_store_params.h @@ -149,6 +149,11 @@ static __always_inline void auxmap__submit_event(struct auxiliary_map *auxmap) { counter->n_drops_buffer++; compute_event_types_stats(auxmap->event_type, counter); } + + /* The event has been handed to the ring buffer (or dropped); release this + * task's auxiliary map entry so the LRU hash is not filled with one entry + * per task that has ever produced an event. See falcosecurity/libs#2719. */ + maps__release_auxiliary_map(); return; } diff --git a/driver/modern_bpf/maps/maps.h b/driver/modern_bpf/maps/maps.h index 3ec27b865..ca4393ec8 100644 --- a/driver/modern_bpf/maps/maps.h +++ b/driver/modern_bpf/maps/maps.h @@ -144,15 +144,40 @@ struct { */ /** - * @brief For every CPU on the system we have an auxiliary - * map where the event is temporally saved before being + * @brief Auxiliary map where the event is temporally saved before being * pushed in the ringbuffer. + * + * This is keyed by `pid_tgid` (not CPU) and backed by an LRU hash to avoid a + * data-corruption race on preemptible kernels (Linux >= 5.11). BPF programs run + * with `migrate_disable()` but are preemptible: a per-CPU scratch buffer can be + * clobbered if a program is preempted mid-write and another program runs on the + * same CPU. Keying by `pid_tgid` gives each in-flight task its own scratch + * entry, since a task can only run on one CPU at a time. See + * falcosecurity/libs#2719. + */ +struct { + __uint(type, BPF_MAP_TYPE_LRU_HASH); + __type(key, uint64_t); + __type(value, struct auxiliary_map); +} auxiliary_maps __weak SEC(".maps"); + +/** + * @brief Single-element template used to initialise a new `auxiliary_maps` + * entry. + * + * `bpf_map_update_elem` on the LRU hash requires a source value to copy from + * when creating a new entry for a task. We cannot build a 128 KB value on the + * BPF stack (512 byte limit), so we keep one element to copy from. Its contents + * are irrelevant (the auxmap is always written before it is read), so it is + * never explicitly initialised. A regular ARRAY is used because a + * BPF_MAP_TYPE_PERCPU_ARRAY element is limited to 32 KB. */ struct { __uint(type, BPF_MAP_TYPE_ARRAY); + __uint(max_entries, 1); __type(key, uint32_t); __type(value, struct auxiliary_map); -} auxiliary_maps __weak SEC(".maps"); +} auxiliary_map_init __weak SEC(".maps"); /** * @brief For every CPU on the system we have a counter diff --git a/userspace/libpman/src/maps.c b/userspace/libpman/src/maps.c index e89186ae8..c5fe69353 100644 --- a/userspace/libpman/src/maps.c +++ b/userspace/libpman/src/maps.c @@ -377,8 +377,18 @@ int pman_mark_single_64bit_syscall(int syscall_id, bool interesting) { } static int size_auxiliary_maps() { - /* We always allocate auxiliary maps from all the CPUs, even if some of them are not online. */ - if(bpf_map__set_max_entries(g_state.skel->maps.auxiliary_maps, g_state.n_possible_cpus)) { + /* `auxiliary_maps` is an LRU hash keyed by `pid_tgid`, holding one scratch + * entry per event currently being built. Entries are released as soon as an + * event is submitted, so at any instant the number of live entries is + * bounded by the number of tasks building an event concurrently (at most + * one per CPU). We size it at 2x the possible CPUs (with a small floor) to + * leave headroom; the LRU reclaims any entries left behind by abandoned + * events. */ + uint32_t aux_entries = g_state.n_possible_cpus * 2; + if(aux_entries < 16) { + aux_entries = 16; + } + if(bpf_map__set_max_entries(g_state.skel->maps.auxiliary_maps, aux_entries)) { pman_print_error("unable to set max entries for 'auxiliary_maps'"); return errno; } diff --git a/userspace/libsinsp/event.cpp b/userspace/libsinsp/event.cpp index de564d756..f5bf86715 100644 --- a/userspace/libsinsp/event.cpp +++ b/userspace/libsinsp/event.cpp @@ -1730,6 +1730,69 @@ void sinsp_evt_param::throw_invalid_len_error(size_t requested_length) const { "parameter raw data: \n" + buffer_to_multiline_hex(param_data, param_len), sinsp_logger::SEV_ERROR); + // Enhanced diagnostics: dump raw event structure to identify the + // root cause of parameter corruption (ROX-33614 investigation). + const scap_evt *raw = m_evt->get_scap_evt(); + const ppm_event_info *evtinfo = m_evt->get_info(); + if(raw && evtinfo) { + std::stringstream diag; + diag << "event diagnostics:" + << " hdr_nparams=" << raw->nparams + << " table_nparams=" << evtinfo->nparams + << " event_len=" << raw->len + << " event_type=" << raw->type + << " hdr_size=" << sizeof(struct ppm_evt_hdr); + libsinsp_logger()->log(diag.str(), sinsp_logger::SEV_ERROR); + + // Dump the length array from the raw event. + // Layout: [ppm_evt_hdr][len0][len1]...[lenN][data0][data1]... + // Each length entry is uint16_t (non-large) or uint32_t (large). + const char *evt_base = reinterpret_cast(raw); + const char *len_array = evt_base + sizeof(struct ppm_evt_hdr); + bool is_large = (evtinfo->flags & EF_LARGE_PAYLOAD) != 0; + uint32_t len_entry_size = is_large ? sizeof(uint32_t) : sizeof(uint16_t); + uint32_t len_array_bytes = raw->nparams * len_entry_size; + + // Dump all param lengths from the raw length array. + std::stringstream lens; + lens << "raw param lengths (" << (is_large ? "large" : "u16") << "):"; + for(uint32_t i = 0; i < raw->nparams && i < PPM_MAX_EVENT_PARAMS; i++) { + uint32_t plen = 0; + if(is_large) { + memcpy(&plen, len_array + i * sizeof(uint32_t), sizeof(uint32_t)); + } else { + uint16_t plen16 = 0; + memcpy(&plen16, len_array + i * sizeof(uint16_t), sizeof(uint16_t)); + plen = plen16; + } + lens << " [" << i << "]=" << plen; + } + libsinsp_logger()->log(lens.str(), sinsp_logger::SEV_ERROR); + + // Dump the raw event header + length array as hex. + size_t hdr_and_lens = sizeof(struct ppm_evt_hdr) + len_array_bytes; + size_t dump_len = std::min(hdr_and_lens, (size_t)256); + libsinsp_logger()->log( + "raw header+lengths (" + std::to_string(dump_len) + " bytes):\n" + + buffer_to_multiline_hex(evt_base, dump_len), + sinsp_logger::SEV_ERROR); + + // Dump the first 128 bytes of the param data region. + const char *data_region = len_array + len_array_bytes; + size_t data_avail = 0; + if(raw->len > hdr_and_lens) { + data_avail = raw->len - hdr_and_lens; + } + size_t data_dump = std::min(data_avail, (size_t)128); + if(data_dump > 0) { + libsinsp_logger()->log( + "param data region (first " + std::to_string(data_dump) + + " of " + std::to_string(data_avail) + " bytes):\n" + + buffer_to_multiline_hex(data_region, data_dump), + sinsp_logger::SEV_ERROR); + } + } + throw sinsp_exception(error_string); }