DF-0813 — hammer2_flush() NULL-deref in retry loop

Summary

sys/vfs/hammer2/hammer2_flush.c retry loop (lines 397-406) dereferences info.parent at lines 403 (hammer2_chain_drop) and 405 (hammer2_chain_ref) without a NULL guard. The setup (382-383) and teardown (434-435) of the same function correctly guard against NULL. If chain->parent becomes NULL during flush_core (via concurrent delete at hammer2_chain.c:3559), the retry loop crashes with a NULL deref panic.

Status: NOT REPRODUCED. The code defect is confirmed real (missing NULL guard, inconsistent with setup/teardown), but the trigger is an extremely tight race between hammer2 flush and concurrent chain deletion. ~15 min of aggressive churn on two hammer2 mounts (with debug trace bit 0x0040 enabled showing the retry-loop body never fired) produced zero panics.

Impact ceiling: NULL-deref panic / DoS (no escalation — fixed-offset deref).

How to reproduce (best-effort — race is probabilistic)

# 1. root: create + mount a hammer2 image
sudo sh setup_h2.sh
# (if label is DATA not DEFAULT: mount_hammer2 /dev/vn0@DATA /mnt/h2 && chmod 1777 /mnt/h2)

# 2. user: build + run the churn harness
./build.sh
H2DIR=/mnt/h2 SECS=300 ./run.sh

# 3. check for panic
dmesg | grep -i panic
# or check the serial console boot.log

The race may require extended churn (many minutes) to win, if winnable at all from pure userspace. Enable sysctl vfs.hammer2.debug=0x40 to observe "LOST CHILD4" trace messages (which fire when the retry-loop body executes at all).

Fix

See fix.diff — adds if (info.parent) guards at lines 403/405, consistent with the setup and teardown. Validated: applies cleanly, compiles, boots (#7), runs the PoC without regression.

DF-0813 — NULL deref in hammer2_flush() retry loop when chain->parent becomes NULL

Verdict

NOT REPRODUCED (race-condition DoS; code defect CONFIRMED real but trigger is an extremely tight race that did not fire in ~15 min of aggressive hammer2 churn).

The code defect (CONFIRMED — path:line)

The finding is correct at the source level. sys/vfs/hammer2/hammer2_flush.c contains a retry loop in hammer2_flush() whose body dereferences info.parent without a NULL guard, while BOTH the setup and teardown of the same function correctly guard against NULL:

Setup (guarded): hammer2_flush.c:382-383

    if ((info.parent = chain->parent) != NULL)
        hammer2_chain_ref(info.parent);

Retry loop body (BUG — unguarded): hammer2_flush.c:403-405

        if (info.parent != chain->parent) {
            ...
            hammer2_chain_drop(info.parent);   /* line 403 — no NULL check */
            info.parent = chain->parent;        /* line 404 — now NULL if chain->parent==NULL */
            hammer2_chain_ref(info.parent);     /* line 405 — derefs NULL -> panic */
        }

Teardown (guarded): hammer2_flush.c:434-435

    if (info.parent)
        hammer2_chain_drop(info.parent);

The inconsistency is unambiguous: setup and teardown guard info.parent against NULL; the loop body does not. If the loop body runs with info.parent == NULL (or chain->parent == NULL after line 404), the calls to hammer2_chain_drop(NULL) / hammer2_chain_ref(NULL) dereference a NULL pointer:

• hammer2_chain_ref(NULL) → atomic_fetchadd_int(&chain->refs, 1) at hammer2_chain.c:260 with chain==NULL → access at offsetof(hammer2_chain_t, refs) below the zero page → fatal trap 12 (page fault).
• hammer2_chain_drop(NULL) → KKASSERT(chain->refs > 0) at hammer2_chain.c:350 → same NULL deref.

Mechanism (theoretically reachable)

For the loop body at line 397 to execute, info.parent != chain->parent must hold on a retry. This requires chain->parent to change during flush_core:

1. flush_core unlocks chain at hammer2_flush.c:662 (hammer2_chain_unlock).
2. During the unlock window (662→665 relock), a concurrent unlink/delete can run _hammer2_chain_delete_helper (hammer2_chain.c:3520), which under chain->core.spin + parent->core.spin (lines 3550-3551) sets chain->parent = NULL at hammer2_chain.c:3559.
3. After relock, flush_core detects chain->parent != parent at line 681, sets retry=1, and returns to the loop.
4. Back at line 397: info.parent (old non-NULL parent) != chain->parent (now NULL) → body enters → line 404 sets info.parent=NULL → line 405 hammer2_chain_ref(NULL) → panic.

The path is reachable in principle. hammer2_chain_delete is called from the VOP unlink/remove xop path (hammer2_xops.c:420,449,641,...), and the flush is driven by user-initiated sync()/fsync().

Why it did NOT reproduce (the race is extremely tight)

Two stress harnesses (churn_h2.c = create/write/fsync/unlink/sync workers; dirstress_h2.c = heavy single-directory topology churn to force indirect-block split/merge + concurrent sync) were run for a cumulative ~15 minutes across two hammer2 mounts (/mnt/h2 vn-backed image and /tmp/h2churn on the root hammer2 fs), with vfs.hammer2.debug=0x0040 enabled (which prints "LOST CHILD4" at hammer2_flush.c:398-402 whenever the retry-loop body fires AT ALL).

Result: zero "LOST CHILD4" events — the retry-loop body at line 397 never executed during any run. This means a chain's parent never changed while a flush was processing it. The race window (flush_core's unlock at :662 → relock at :665) is only a few lock operations wide, and the hammer2 flush/delete serialization prevented the overlap from occurring in practice. Zero panics on both the unpatched #0 and patched #7 kernels.

Impact ceiling

A NULL deref at a fixed offset (atomic_fetchadd_int at &NULL->refs, or KKASSERT on NULL->refs) is a pure DoS (kernel panic) — no memory corruption, no attacker-controlled write target, no escalation path. The panic would crash the system but cannot be leveraged for code execution or privilege escalation.

Realistic trigger: an unprivileged user with write access to a mounted hammer2 filesystem doing heavy concurrent modify/unlink + sync operations. The race is probabilistic and very tight; it may require extended churn to win (if winnable at all from pure userspace).

Fix (fix.diff)

The fix adds NULL guards in the retry loop body, making it consistent with the setup (line 382-383) and teardown (line 434-435):

--- a/sys/vfs/hammer2/hammer2_flush.c
+++ b/sys/vfs/hammer2/hammer2_flush.c
@@ -400,9 +400,11 @@
                    info.parent, chain, chain->parent);
            }
-           hammer2_chain_drop(info.parent);
+           if (info.parent)
+               hammer2_chain_drop(info.parent);
            info.parent = chain->parent;
-           hammer2_chain_ref(info.parent);
+           if (info.parent)
+               hammer2_chain_ref(info.parent);
        }

This is a minimal, targeted defense-in-depth fix: if chain->parent becomes NULL during a retry, the loop simply drops the old parent (if any) and sets info.parent=NULL without crashing; subsequent flush_core and teardown handle a NULL parent correctly (they all guard with if (parent) / if (info.parent)).

Fix validation (Phase 8)

• fix.diff applies cleanly: git apply --check and patch -p1 both succeed.
• Patched kernel compiles: make -j6 nativekernel KERNCONF=X86_64_GENERIC produced kernel.stripped (15.7 MB) and kernel.debug (119 MB) with zero compile errors. hammer2_flush.o rebuilt successfully.
• Patched kernel boots: kern.version = 6.5-DEVELOPMENT #7 (today's ts).
• PoC re-run on patched kernel: both harnesses ran for ~3 min with no panic, no regression, guest healthy.
• fix_status: not_testable — since the race did not fire on EITHER the unpatched baseline or the patched kernel, a behavioral before/after comparison is not possible. The fix is validated at the compile + boot + code-inspection level: the NULL-deref path at lines 403/405 is definitively closed.

PoC changes

• Authored churn_h2.c (create/write/fsync/unlink/sync churn harness).
• Authored dirstress_h2.c (directory-topology-stress variant for indirect-block churn).
• Authored setup_h2.sh / teardown_h2.sh (root: create vn-backed hammer2 image, newfs, mount).
• Authored fix.diff (NULL guards in the retry loop body).

Kernel references (confirmed during verification)

• sys/vfs/hammer2/hammer2_flush.c:397 — retry loop condition (info.parent != chain->parent)
• sys/vfs/hammer2/hammer2_flush.c:403 — unguarded hammer2_chain_drop(info.parent)
• sys/vfs/hammer2/hammer2_flush.c:405 — unguarded hammer2_chain_ref(info.parent) ← NULL deref
• sys/vfs/hammer2/hammer2_flush.c:382-383 — setup (correctly guarded)
• sys/vfs/hammer2/hammer2_flush.c:434-435 — teardown (correctly guarded)
• sys/vfs/hammer2/hammer2_flush.c:662 — hammer2_chain_unlock(chain) (race window open)
• sys/vfs/hammer2/hammer2_flush.c:665 — hammer2_chain_lock(chain) (race window close)
• sys/vfs/hammer2/hammer2_flush.c:681-689 — parent-change detection, sets retry=1
• sys/vfs/hammer2/hammer2_chain.c:3559 — chain->parent = NULL in delete_helper
• sys/vfs/hammer2/hammer2_chain.c:260 — atomic_fetchadd_int(&chain->refs,1) in chain_ref
• sys/vfs/hammer2/hammer2_chain.c:350 — KKASSERT(chain->refs > 0) in chain_drop