Skip to content

Fix ZFS bugs (diskspace.plugin) - #22188

Merged
thiagoftsm merged 1 commit into
netdata:masterfrom
thiagoftsm:fix_zfs
Apr 13, 2026
Merged

Fix ZFS bugs (diskspace.plugin)#22188
thiagoftsm merged 1 commit into
netdata:masterfrom
thiagoftsm:fix_zfs

Conversation

@thiagoftsm

@thiagoftsm thiagoftsm commented Apr 10, 2026

Copy link
Copy Markdown
Contributor
Summary

This PR fixes the following bug:

#1 is_zfs_filesystem [0x5DDC1BC1E2C0] (/src/collectors/diskspace.plugin/plugin_diskspace.c:355)
#2 zfs_collect_pool_capacities [0x5DDC1BC1E2C0] (/src/collectors/diskspace.plugin/plugin_diskspace.c:387)
#3 diskspace_main [0x5DDC1BC1E2C0] (/src/collectors/diskspace.plugin/plugin_diskspace.c:1176)
#4 nd_thread_starting_point [0x5DDC1BD803B1] (/src/libnetdata/threads/threads.c:367)
#5 <unknown> [0x79ADBE6A71F4]

Details about the fixes:

Fixes Bug 1 — NULL deref in basic_mountinfo_create_and_copy (line 165)

// Before
bmi->filesystem = strdupz(mi->filesystem);

// After
bmi->filesystem = mi->filesystem ? strdupz(mi->filesystem) : NULL;
mount_source already had this guard; filesystem was missing it.


Bug 2 (root cause of the crash) — zfs_collect_pool_capacities removed
entirely

The function was calling statvfs() on ZFS pool mounts:

  • inside slow_mountinfo_mutex — blocking the entire collection loop and
    starving the slow worker thread
  • without slow-mount protection (no timeout, no slow flag)
  • on degraded/exporting/hung ZFS pools this can trigger kernel-level memory
    corruption, making mi->filesystem an invalid pointer by the next iteration
    → strcmp crash

Replaced with zfs_cache_pool_capacity() — a lightweight helper that
piggy-backs on the statvfs() call that do_disk_space_stats and
do_slow_disk_space_stats already make. No extra syscall, no extra mutex
hold time.

Test Plan
  1. Compile this branch;
  2. Create a ZFS pool using dd;
  3. Start netdata;
  4. Take a look on charts and verify everything is running as expected;
  5. Stop netdata;
  6. Check coredump does not happen again
Additional Information
For users: How does this change affect me?

Summary by cubic

Fixes ZFS-related coredumps in diskspace.plugin by guarding NULL filesystem fields and replacing the blocking pool-capacity collector with a cache fed by existing statvfs calls. This prevents strcmp crashes and avoids blocking the collection loop on degraded or exporting ZFS pools.

  • Bug Fixes
    • Add NULL check when copying filesystem in basic_mountinfo_create_and_copy.
    • Remove zfs_collect_pool_capacities; add a lightweight zfs_cache_pool_capacity that updates after successful statvfs in the fast/slow paths (no extra syscalls or lock time).
    • Only cache pool mounts (no '/' in mount_source) and skip updates when the cache is fresh.
    • Initialize LXC detection once at startup; LXC environments bypass the ZFS exclusion heuristic.

Written for commit 91ce11f. Summary will update on new commits.

@github-actions github-actions Bot added area/collectors Everything related to data collection collectors/diskspace labels Apr 10, 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 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.
Architecture diagram
sequenceDiagram
    participant Startup as Startup (diskspace_main)
    participant Main as Main Thread (Fast Path)
    participant Worker as Slow Worker (Thread)
    participant OS as Kernel (statvfs)
    participant Cache as ZFS Cache (Dictionary)

    Note over Startup,Cache: Initialization Phase
    Startup->>OS: NEW: is_lxcfs_proc_mounted()
    OS-->>Startup: LXC status (set once)

    Note over Startup,Cache: Runtime Collection Loop
    
    rect rgb(240, 240, 240)
    Note right of Main: Fast Path: Processes responsive mounts
    Main->>Main: basic_mountinfo_create_and_copy()
    Note right of Main: NEW: NULL guard on mi->filesystem copy
    
    Main->>OS: statvfs(mount_point)
    OS-->>Main: buff_statvfs
    
    Main->>Cache: NEW: zfs_cache_pool_capacity(buff_statvfs)
    opt If mount is ZFS Pool (no '/' in source)
        Cache->>Cache: Update capacity & timestamp
    end
    
    Main->>Cache: should_exclude_zfs()
    Cache-->>Main: Decision (based on heuristic)
    end

    rect rgb(230, 240, 250)
    Note right of Worker: Slow Path: Processes network/hung mounts
    Worker->>OS: statvfs(mount_point)
    alt statvfs success
        OS-->>Worker: buff_statvfs
        Worker->>Cache: NEW: zfs_cache_pool_capacity(buff_statvfs)
        Note right of Worker: Updates cache from worker thread
        Worker->>Cache: should_exclude_zfs()
        Cache-->>Worker: Decision
    else statvfs timeout/error
        OS-->>Worker: error
    end
    end

    Note over Main,Cache: CHANGED: No separate blocking ZFS pass in Main Loop. 
    Note over Main,Cache: Capacity data now flows from existing statvfs calls.
Loading

@thiagoftsm
thiagoftsm requested a review from stelfrag April 10, 2026 23:26
@thiagoftsm
thiagoftsm marked this pull request as ready for review April 10, 2026 23:26
@stelfrag
stelfrag requested a review from Copilot April 11, 2026 07:30

Copilot AI 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.

Pull request overview

This PR addresses ZFS-related crashes in diskspace.plugin by hardening mountinfo copying against NULL fields and removing the extra ZFS pool-capacity statvfs pass that could block collection and contribute to instability.

Changes:

  • Guard mi->filesystem when copying into basic_mountinfo to avoid NULL dereferences.
  • Replace zfs_collect_pool_capacities() with zfs_cache_pool_capacity(), updating the ZFS pool-capacity cache only after already-successful statvfs() calls in the fast/slow paths.
  • Initialize LXC detection once during diskspace_main() startup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@thiagoftsm
thiagoftsm merged commit d4e069f into netdata:master Apr 13, 2026
156 checks passed
@thiagoftsm
thiagoftsm deleted the fix_zfs branch April 13, 2026 02:08
stelfrag pushed a commit to stelfrag/netdata that referenced this pull request Apr 14, 2026
@stelfrag stelfrag mentioned this pull request Apr 14, 2026
Ferroin pushed a commit that referenced this pull request Apr 14, 2026
nedi-app Bot pushed a commit that referenced this pull request Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/collectors Everything related to data collection collectors/diskspace

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants