Skip to content

Patch release 2.10.3#22249

Merged
Ferroin merged 4 commits into
netdata:v2.10from
stelfrag:patch_release
Apr 27, 2026
Merged

Patch release 2.10.3#22249
Ferroin merged 4 commits into
netdata:v2.10from
stelfrag:patch_release

Conversation

@stelfrag

@stelfrag stelfrag commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator
Summary

Summary by cubic

Fixes a PID shared-memory pool leak and 100% CPU spin in ebpf.plugin, splits dynamic-config job name validation by domain, switches SNMP uptime to snmpEngineTime, and removes an obsolete PowerStore field.

  • Bug Fixes

    • Separated SHM lookup from allocation; aggregation uses a non-allocating lookup. Zeroed new slots and sweep module bits on exit to release stale entries.
    • Prevented infinite loops when SHM is full by always advancing BPF map iteration; always delete dead socket tuples from the kernel map.
    • Fixed PID liveness checks to only treat ESRCH as dead; cleaned SHM/semaphore lifecycle with shm_unlink/sem_unlink and wiped the region on init.
    • Made SNMP-FRAMEWORK-MIB::snmpEngineTime the primary systemUptime in go.d/snmp; keeps the same metric name and HR-MIB as fallback.
    • Removed ExtraDetails from go.d/powerstore Hardware to match the API and avoid decode errors.
  • Migration

    • If you implement dyncfg.Callbacks, add ValidateJobName(name string) error.
    • Use JobNameRuleStrict for collectors; use JobNameRuleAllowDots for service discovery, secret store, and vnode names.

Written for commit 58b6d16. Summary will update on new commits. Review in cubic

@github-actions github-actions Bot added area/collectors Everything related to data collection collectors/go.d area/go labels Apr 22, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 12 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.
Architecture diagram
sequenceDiagram
    participant Source as Command Source
    participant Handler as dyncfg.Handler
    participant Domain as Domain Callbacks (Collector, SD, Vnode, etc.)
    participant Rules as Validation Rules

    Note over Source,Rules: Dynamic Configuration Job Creation Flow

    Source->>Handler: CmdAdd(fn)
    Handler->>Domain: ExtractKey(fn)
    Domain-->>Handler: key, name

    Handler->>Domain: NEW: ValidateJobName(name)
    
    alt Domain: Collector
        Domain->>Rules: NEW: JobNameRuleStrict(name)
        Note right of Rules: Rejects spaces, colons, AND dots
        Rules-->>Domain: error/nil
    else Domain: SD, SecretStore, or Vnode
        Domain->>Rules: NEW: JobNameRuleAllowDots(name)
        Note right of Rules: Rejects spaces and colons ONLY
        Rules-->>Domain: error/nil
    end

    Domain-->>Handler: Validation Result

    alt Invalid Name (Validation Failed)
        Handler-->>Source: 400 Bad Request (invalid job name)
    else Valid Name
        Handler->>Domain: ParseAndValidate(fn, name)
        
        alt Parse Success
            Domain-->>Handler: Config object
            Handler->>Domain: Start(Config)
            Handler-->>Source: 200/201 Success
        else Parse Failure
            Domain-->>Handler: error
            Handler-->>Source: 400/500 Error
        end
    end
Loading

…spin (netdata#22232)

* ebpf.plugin: fix PID accounting shared-memory pool leak and 100% CPU spin

The eBPF plugin's per-PID shared-memory pool (32,768 slots backing
/dev/shm/netdata_shm_integration_ebpf) monotonically fills on any host
with normal process churn, and once the pool is exhausted each module's
apps-read loop spins one CPU core at 100% indefinitely.

ROOT CAUSE

netdata_ebpf_get_shm_pointer_unsafe() is an allocate-AND-set-bit API,
but it is called from two incompatible contexts:

  1. The per-module BPF map iteration (ebpf_read_*_apps_table), which
     is paired with netdata_ebpf_reset_shm_pointer_unsafe() via the
     kill(pid, 0) == ESRCH branch. This is a correct producer/consumer.

  2. The apps and cgroup aggregation helpers (*_sum_pids,
     *_update_*_cgroup, *_resume_apps_data), which iterate live-PID
     lists built from /proc and cgroup snapshots and call the same
     allocating accessor. These paths have NO reset counterpart.

Every second, for every live PID seen in /proc or a tracked cgroup,
every enabled eBPF module unconditionally sets its bit in the shm
slot for that PID. A dead PID's bit is only cleared by the owning
module's BPF-map iteration - but the BPF map's entry was already
deleted in the same cycle (by bpf_map_delete_elem inside
reset_shm_pointer_unsafe), so the next iteration cannot find the PID
and the bit is stuck forever. The slot therefore never reaches
threads == 0 and is never compacted out. Over 15h on a workstation
with ~1,300 live PIDs the pool reaches 100% with ~96% stale entries.

Once the pool is full, get_shm_pointer_unsafe() returns NULL. Every
apps_read loop had a bare `continue;` after the NULL check, which
skipped the `key = next_key;` advance, so bpf_map_get_next_key()
re-returned the same key forever and the thread spun at 100% CPU.

FIXES

  A. Split allocation from lookup. Introduce
     netdata_ebpf_lookup_shm_pointer_unsafe(pid) that returns the
     existing slot or NULL and sets no bit. Replace the 16 aggregation
     call sites with the new API. Keep get_shm_pointer_unsafe() at the
     8 BPF-map iteration call sites where allocation is legitimate and
     paired with reset. Aggregation paths additionally gate on the
     module bit being set so they do not consume zero-initialised
     per-module sub-structs.

  B. Replace `continue;` with `goto end_*_loop;` in all 8 apps-read
     loops so `key = next_key;` always runs, preventing the infinite
     loop even when the pool is legitimately exhausted.

  C. Zero freshly-allocated slots. ebpf_find_or_create_index_pid()
     now memset()s the slot on the create branch, so compacted tails
     and PID reuse cannot leak stale bits or stale per-module
     counters into new occupants.

  D. Use the correct kill() error check. Five modules used
     `if (kill(pid, 0))`, which treats EPERM (cross-UID processes) as
     "process is dead" and erroneously deletes the live process's
     kernel BPF map entry and zeroes its shm data. Switched all of
     them to `if (kill(pid, 0) == -1 && errno == ESRCH)`, matching
     the two modules that already did it correctly. Also applied to
     ebpf_parse_proc_files().

  E. Collapse ebpf_reset_specific_pid_data() to its effective body.
     thread_collecting only ever has NETDATA_EBPF_PIDS_PROC_FILE set,
     so the `idx < PROC_FILE` switch was dead code. The function now
     just forwards to ebpf_del_pid_entry(). The only other reader of
     that bitmask, ebpf_release_pid_data(), had zero callers and was
     removed.

  F. Sweep the shm pool on module exit. New helper
     netdata_ebpf_sweep_shm_for_module_unsafe() clears this module's
     bit across every shm slot and lets the existing compaction path
     release slots that become empty. All eight per-PID modules now
     call it under the shm semaphore in their *_exit path, so runtime
     module restarts no longer leak bits.

VERIFICATION

  - Root cause was captured on a live ebpf.plugin pinned at 64% CPU.
    gdb showed an EBPF_READ_FD thread permanently alternating
    bpf_map_get_next_key/bpf_map_lookup_elem at ebpf_fd.c:770-774,
    and ebpf_stat_values = {total = 32768, current = 32768} while
    ps showed 1,307 live PIDs.

  - The diagnosis was independently confirmed by a parallel review
    (codex, qwen, glm, minimax, opus), all converging on the
    alloc-without-reset asymmetry in the aggregation paths.

  - The modified tree builds cleanly (`cmake --build build --target
    ebpf.plugin`). No new warnings in the changed files; only the
    pre-existing vendored-Judy stringop-overflow warnings remain.

* ebpf.plugin: wipe shm on init, unlink on cleanup, drop unused out-param

Close two residual issues around the per-PID shared-memory integration
pool, spotted post-merge:

1. The POSIX shm object `netdata_shm_integration_ebpf` was neither
   truncated on open nor unlinked on shutdown. `shm_open(..., O_CREAT)`
   on an existing object just reopens it, and `ftruncate` to the
   previous size is a no-op, so the 14 MB mapped region retained the
   previous plugin run's bytes. The slot-allocation path memsets each
   new slot, so this was not a correctness bug — but every slot in
   `[current, total)` kept stale PID numbers and per-module counters
   until system reboot. Now:
   - `netdata_integration_initialize_shm()` memsets the whole mapped
     region immediately after `nd_mmap`, so the next plugin instance
     always starts from a clean slab.
   - `netdata_integration_cleanup_shm()` calls `shm_unlink` so a
     clean shutdown removes the backing object from `/dev/shm`.

2. Drop the unused `bool *created` out-parameter on
   `ebpf_find_or_create_index_pid()`. It was added when the memset
   was factored out of the allocation path; since the memset now
   lives inside the function, no caller needs to know whether the
   slot was freshly allocated. Reported by Copilot on PR netdata#22232.

* ebpf.plugin: address Copilot findings on 22232

Four residual issues spotted by Copilot after the initial round of
changes. All valid.

1. get_shm_pointer_unsafe() no longer bails out when the pool is full.
   The caller path for an already-tracked PID must still reach its
   slot so module bits can be updated or cleared. The inner
   ebpf_find_or_create_index_pid() already returns existing slots
   unconditionally and only rejects *new* allocations when full, so
   the redundant outer guard was both dead weight and actively
   harmful — it made existing PIDs unreachable the moment the pool
   filled.

2. Semaphore lifecycle. netdata_integration_initialize_shm() now
   sem_unlinks NETDATA_EBPF_SHM_INTEGRATION_NAME before sem_open, and
   netdata_integration_cleanup_shm() sem_unlinks on the way out. If
   a previous plugin instance crashed while holding the semaphore,
   sem_open(O_CREAT) on the same name would reuse the existing
   semaphore at its last value (O_CREAT's initial-value argument is
   ignored when the semaphore already exists), and the next run
   would spin on sem_timedwait() timeouts forever. Unlinking
   guarantees the initial value of 1 is honoured.

3. Socket apps iteration: the kernel-map delete for a dead socket
   tuple used to run inside `if (local_pid) { ... }`. When the shm
   pool is full and local_pid is NULL, `deleted` sockets were left
   in the BPF map to be re-iterated every cycle, growing the map
   unbounded. Moved bpf_map_delete_elem(fd, &key) outside the
   local_pid branch; it is conditioned only on `deleted`, which
   captures the socket's own freshness signal and is independent of
   shm pool state. (For SOCKET_IDX the generic
   reset_shm_pointer_unsafe() intentionally skips the delete, since
   the socket map is keyed by a tuple, not by pid — comment added.)

(cherry picked from commit bd38550)
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
9.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@Ferroin Ferroin marked this pull request as ready for review April 27, 2026 13:24
Copilot AI review requested due to automatic review settings April 27, 2026 13:24
@Ferroin Ferroin merged commit f635797 into netdata:v2.10 Apr 27, 2026
140 of 146 checks passed
@stelfrag stelfrag removed the request for review from Copilot April 27, 2026 13:48
@stelfrag stelfrag deleted the patch_release branch April 27, 2026 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants