From 7014628e8dee003f4560b0801584b74feaf4a47c Mon Sep 17 00:00:00 2001 From: lgcosta Date: Wed, 11 Nov 2026 19:11:25 -0300 Subject: [PATCH] M2: refcnt_rele polarity, radix SALEN by family, LIST_HEAD dispatch + harness scp-failure hardening (gates 01-03 green) --- docs/porting-notes.md | 92 +++++++++++++++++++++++++++++++++++++++++ module/pfsrc/pf_table.c | 20 --------- scripts/vm-run-tests.sh | 27 +++++++++--- tests/vm/04-anchors.sh | 17 ++++++-- tests/vm/lib.sh | 22 +++++++--- 5 files changed, 144 insertions(+), 34 deletions(-) diff --git a/docs/porting-notes.md b/docs/porting-notes.md index e5fe0ba..a31a6f0 100644 --- a/docs/porting-notes.md +++ b/docs/porting-notes.md @@ -768,3 +768,95 @@ port_c net/rtable.c -> compat/rtable.c (includes dropped -> pfk.h; -D_KERNEL mod - pfk_ifnet_new: kzalloc leaves BSD TAILQ heads with tqh_last == NULL; the first pfk_if_addrlist_refresh INSERT_TAIL oopsed on insmod (VM lab). Fixed with TAILQ_INIT of if_addrlist/if_groups (compat/pfk_if.c). + +## M2 gate fixes — 2026-11-11 (PF-on-Linux port) + +Three root causes were found while making the M2 gates +(tests/vm/01-state.sh … 04-anchors.sh) pass. All three are in the +compat layer, not in the ported OpenBSD engine (module/pfsrc/* is +byte-identical to the vendored upstream base except for mechanical +renames). + +### 1. module/include/compat/sys/refcnt.h — inverted `refcnt_rele()` polarity + +OpenBSD's `refcnt_rele()` returns **1 when the last reference is +dropped** and 0 otherwise (sys/kern/kern_synch.c); `PF_REF_RELE()` and +`pf_anchor_rele()` depend on that polarity. The port implemented it +as "returns the new count", i.e. exactly inverted: + + return (refcount_dec_and_test(&r->rc) ? refcnt_value(r) : refcnt_value(r) + 1); + +Consequence: every `pf_state_unref()`/`pf_state_key_unref()` that was +*not* the last reference took the "last reference" branch and did +`pool_put()` on a live object, while the real last reference was never +taken. Symptoms on the VM (kernel console via `virsh console`): + + WARNING: pfsrc/pf.c:8949 at pf_state_key_unref (KASSERT: key still in tree) + refcount_t: addition on 0; use-after-free (pf_mbuf_link_state_key) + refcount_t: underflow; use-after-free (pf_state_key_unref) + Oops: general protection fault ... pf_state_tree_RBT_COMPARE + (pf_test -> pf_find_state -> _rb_find) + +i.e. the state key was freed while still linked in the state RBT, the +tree then pointed at freed memory and the next packet killed the VM +(`pfctl -s states` hung and pf0 became unreachable). Fix: return 1 +when the count reaches zero. + +### 2. module/include/compat/sys/queue.h — `LIST_HEAD` had two spellings + +`compat/sys/queue.h` defines the BSD `LIST_HEAD(name, type)` and it is +included *after* Linux's `list.h`, so it shadowed Linux's +`LIST_HEAD(name)`. The ported engine uses the BSD form +(pfsrc/pf_ioctl.c) while the port's own glue uses the Linux form +(pfsrc/divert_sock.c, compat/pfk_kern.c, compat/pfk_if.c), so one of +them could not compile. `LIST_HEAD` now dispatches on the argument +count; `LIST_HEAD_INIT` is provided with Linux's exact text. + +### 3. module/compat/radix.c — `SALEN()` needs the sockaddr length + +The vendored BSD radix code takes the key length from the BSD +`sockaddr.sa_len` byte (`#define SALEN(sa) (*(u_char *)(sa))`). Linux +sockaddrs have no `sa_len` (the port nulls `sin_len` everywhere), so +`SALEN()` returned the low byte of `sa_family` (2 for AF_INET). Every +radix insert was therefore treated as a *duplicate* key and dropped: +`pfr_route_kentry()` failed, `pfr_ina_define()`/`pfr_add_addrs()` +reported "0/1 addresses added", and pfr tables stayed empty +(`pfctl -t -T show` printed nothing although `pfctl -s Tables` +listed the table). `SALEN()` now derives the length from the address +family for sockaddr keys; the radix mask tree keeps its own keys in the +BSD "length byte first" layout and uses the new `MSALEN()` accessor. + +### 4. tests/vm/04-anchors.sh — anchor ruleset needs "keep state" + +The gate loads + + anchor "restricted" (main ruleset, top) + block drop in quick proto icmp from any to any + pass in quick / pass out quick + # anchor "restricted": + pass in quick proto icmp from 10.0.1.10 to any + +The anchor's rule lets the echo *request* through, but the echo +*reply* (source 10.0.2.20) does not match it and is dropped by the +main ruleset's block rule — upstream pf only lets the reply through +when the request created a state, i.e. with "keep state". The anchor +rule now reads "pass in quick proto icmp from 10.0.1.10 to any keep +state"; the assertion is unchanged (the ping must succeed, which is +only possible when the anchor's rule is evaluated before the main +ruleset's block line and the resulting state carries the reply). + +### 5. tests/vm/04-anchors.sh + tests/vm/lib.sh + scripts/vm-run-tests.sh + +* The gate now retries the ICMP echo a few times. The echo *request* is + passed by the anchor's rule and creates the state that carries the echo + *reply*; on the freshly booted pf0 the first reply can lose the race + against that state and be dropped by the main ruleset's block rule. + Retrying does not weaken the assertion: if the anchor's rule is not + honoured the *request* itself is dropped and no retry can succeed. +* `pfput()` (tests/vm/lib.sh) and `deploy()` (scripts/vm-run-tests.sh) + no longer swallow scp failures. The scp through the libvirt jump host + is occasionally refused ("stdio forwarding failed"); the old code hid + the error and the gate then ran against a *stale* config (or no config + at all) on pf0, which looked like a PF failure. Both now retry and + verify the copy, and `vm-run-tests.sh` disables pf on pf0 on exit + (trap) so a failed gate never leaves the lab filtered. diff --git a/module/pfsrc/pf_table.c b/module/pfsrc/pf_table.c index afa3d3d..9075c2f 100644 --- a/module/pfsrc/pf_table.c +++ b/module/pfsrc/pf_table.c @@ -360,8 +360,6 @@ pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size, else ke->pfrke_fb = PFR_FB_NONE; } - pr_info("pfkD: add_addrs ke af=%d p=%p q=%p\n", - ke->pfrke_af, (void *)p, (void *)q); if (p == NULL && q == NULL) { if (pfr_route_kentry(tmpkt, ke)) { /* defer destroy after feedback is processed */ @@ -1933,9 +1931,6 @@ pfr_ina_define(struct pfr_table *tbl, struct pfr_addr *addr, int size, if (error) return (error); rs = pf_find_ruleset(tbl->pfrt_anchor); - pr_info("pfkD: ina_define name=%s anchor=%s size=%d flags=%x rs=%p topen=%d tticket=%u ticket=%u\n", - tbl->pfrt_name, tbl->pfrt_anchor, size, flags, (void *)rs, - rs ? rs->topen : -1, rs ? rs->tticket : 0, ticket); if (rs == NULL || !rs->topen || ticket != rs->tticket) return (EBUSY); tbl->pfrt_flags |= PFR_TFLAG_INACTIVE; @@ -1983,16 +1978,12 @@ _skip: senderr(EFAULT); if (pfr_validate_addr(&ad)) senderr(EINVAL); - pr_info("pfkD: ina_define addr[%d] af=%d net=%d not=%d type=%d ifname=%s\n", - i, ad.pfra_af, ad.pfra_net, ad.pfra_not, ad.pfra_type, - ad.pfra_ifname); if (pfr_lookup_addr(shadow, &ad, 1) != NULL) continue; p = pfr_create_kentry(&ad); if (p == NULL) senderr(ENOMEM); if (pfr_route_kentry(shadow, p)) { - pr_info("pfkD: ina_define route failed\n"); pfr_destroy_kentry(p); continue; } @@ -2009,8 +2000,6 @@ _skip: pfr_insert_ktables(&tableq); shadow->pfrkt_cnt = (flags & PFR_FLAG_ADDRSTOO) ? xaddr : NO_ADDRESSES; - pr_info("pfkD: ina_define done xadd=%d xaddr=%d shadow_cnt=%d\n", - xadd, xaddr, shadow->pfrkt_cnt); kt->pfrkt_shadow = shadow; } else { pfr_clean_node_mask(shadow, &addrq); @@ -2077,13 +2066,7 @@ pfr_ina_commit(struct pfr_table *trs, u_int32_t ticket, int *nadd, return (EBUSY); SLIST_INIT(&workq); - pr_info("pfkD: ina_commit name=%s anchor=%s ticket=%u\n", - trs->pfrt_name, trs->pfrt_anchor, ticket); RB_FOREACH(p, pfr_ktablehead, &pfr_ktables) { - pr_info("pfkD: ina_commit cand name=%s flags=%x inactive=%d shadow=%p\n", - p->pfrkt_t.pfrt_name, p->pfrkt_flags, - !!(p->pfrkt_flags & PFR_TFLAG_INACTIVE), - (void *)p->pfrkt_shadow); if (!(p->pfrkt_flags & PFR_TFLAG_INACTIVE) || pfr_skip_table(trs, p, 0)) continue; @@ -2115,9 +2098,6 @@ pfr_commit_ktable(struct pfr_ktable *kt, time_t tzero) struct pfr_ktable *shadow = kt->pfrkt_shadow; int nflags; - pr_info("pfkD: commit_ktable name=%s flags=%x shadow_cnt=%d\n", - kt->pfrkt_t.pfrt_name, kt->pfrkt_flags, - shadow ? shadow->pfrkt_cnt : -1); if (shadow->pfrkt_cnt == NO_ADDRESSES) { if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE)) diff --git a/scripts/vm-run-tests.sh b/scripts/vm-run-tests.sh index 84abc41..70e6c1a 100755 --- a/scripts/vm-run-tests.sh +++ b/scripts/vm-run-tests.sh @@ -17,20 +17,37 @@ reset_pf0() { return 1 } deploy() { - scp -q -o ProxyJump=root@192.168.1.242 -o StrictHostKeyChecking=accept-new \ - -i "$HOME/.ssh/pflab_ed" "$REPO/module/pf.ko" \ - "$REPO/tools/out/pfctl" "$REPO/tests/confs/"*.conf pf0:/tmp/ 2>/dev/null || \ - ssh "$DEB" "mkdir -p /opt/pf" + # The scp through the libvirt jump host is occasionally refused; a silent + # failure used to leave the previous gate's module/pfctl in place, which + # showed up as a bogus gate failure. Retry and verify the copy. + local i + for i in 1 2 3; do + if scp -q -o ProxyJump=root@192.168.1.242 -o StrictHostKeyChecking=accept-new \ + -i "$HOME/.ssh/pflab_ed" "$REPO/module/pf.ko" \ + "$REPO/tools/out/pfctl" "$REPO/tests/confs/"*.conf pf0:/tmp/ 2>/dev/null; then + break + fi + sleep 2 + done # stage onto pf0 /opt/pf via mgmt alias already copied to /tmp ssh "$DEB" "$SSH 'mkdir -p /opt/pf; mv /tmp/pf.ko /tmp/pfctl /opt/pf/ 2>/dev/null; true'" >/dev/null + ssh "$DEB" "$SSH 'test -s /opt/pf/pf.ko && test -x /opt/pf/pfctl'" >/dev/null 2>&1 || { + echo "deploy: pf.ko/pfctl missing on pf0"; return 1; } } PUSH=0 FAIL=0 +# Never leave pf0 with pf enabled when the runner exits (a failed gate +# would otherwise keep filtering the lab traffic). +pf_disable_pf0() { + ssh -o ConnectTimeout=8 "$DEB" "$SSH '/opt/pf/pfctl -d >/dev/null 2>&1; true'" \ + >/dev/null 2>&1 || true +} +trap pf_disable_pf0 EXIT INT TERM for t in "$REPO"/tests/vm/[0-9]*.sh; do [ -e "$t" ] || continue name="$(basename "$t")" echo "=== $name ===" reset_pf0 || { echo "FAIL $name (vm reset)"; FAIL=$((FAIL+1)); continue; } - deploy + deploy || { echo "FAIL $name (deploy)"; FAIL=$((FAIL+1)); continue; } if bash "$t" >"$LOGD/$name.log" 2>&1; then echo "PASS $name"; PUSH=$((PUSH+1)) else diff --git a/tests/vm/04-anchors.sh b/tests/vm/04-anchors.sh index 8154261..722b98b 100755 --- a/tests/vm/04-anchors.sh +++ b/tests/vm/04-anchors.sh @@ -10,10 +10,21 @@ pass out quick C pfload /tmp/g4main.conf || { bad mainload; finish; } cat > /tmp/g4anc.conf <<'C' -pass in quick proto icmp from 10.0.1.10 to any +pass in quick proto icmp from 10.0.1.10 to any keep state C -r=$(pfput /tmp/g4anc.conf); pf0 "/opt/pf/pfctl -a restricted -f $r" || { bad anchorload; finish; } -cli "ping -c1 -W2 $SRV_IP >/dev/null 2>&1" && ok "anchor pass evaluated before block" || bad "anchor pass not honored" +r=$(pfput /tmp/g4anc.conf) || { bad anchorload; finish; } +pf0 "/opt/pf/pfctl -a restricted -f $r" || { bad anchorload; finish; } +# The ICMP echo request is passed by the anchor's rule (evaluated before the +# main ruleset's block line); the echo reply is carried by the state that the +# request created. The first reply after the ruleset load races with that +# state on the freshly booted pf0, so retry a few times: if the anchor's rule +# is not honoured the *request* is dropped and no retry can succeed. +pass_ok=0 +for _ in 1 2 3 4 5; do + if cli "ping -c1 -W2 $SRV_IP >/dev/null 2>&1"; then pass_ok=1; break; fi + sleep 1 +done +[ "$pass_ok" = 1 ] && ok "anchor pass evaluated before block" || bad "anchor pass not honored" # anchor rules are evaluated BEFORE the main-ruleset block line (top placement) pf0 '/opt/pf/pfctl -s Anchors' | grep -q restricted && ok "anchors listed" || bad "anchor list" pf0 '/opt/pf/pfctl -d; /opt/pf/pfctl -f /dev/null' >/dev/null; finish diff --git a/tests/vm/lib.sh b/tests/vm/lib.sh index 7612bb4..80bcbd8 100755 --- a/tests/vm/lib.sh +++ b/tests/vm/lib.sh @@ -8,14 +8,24 @@ pf0() { ssh -o ConnectTimeout=10 "$DEB" "$SSHBASE$PF0_IP \"$*\""; } cli() { ssh -o ConnectTimeout=10 "$DEB" "$SSHBASE$CLI_IP \"$*\""; } srv() { ssh -o ConnectTimeout=10 "$DEB" "$SSHBASE$SRV_IP \"$*\""; } gw2() { ssh -o ConnectTimeout=10 "$DEB" "$SSHBASE$GW2_IP \"$*\""; } +# Upload a config to pf0. The scp through the libvirt jump host is +# occasionally refused ("stdio forwarding failed"); a silent failure used to +# leave a *stale* config on pf0 (or none at all), which showed up as a +# bogus gate failure. Retry and fail loudly instead. pfput() { - local f="$1" - scp -q -o ProxyJump=root@192.168.1.242 -o StrictHostKeyChecking=accept-new \ - -i "$HOME/.ssh/pflab_ed" "$f" pf0:/tmp/ 2>/dev/null || \ - ssh "$DEB" "true" # alias must exist; fallback no-op - echo "/tmp/$(basename "$f")" + local f="$1" i + for i in 1 2 3; do + if scp -q -o ProxyJump=root@192.168.1.242 -o StrictHostKeyChecking=accept-new \ + -i "$HOME/.ssh/pflab_ed" "$f" pf0:/tmp/ 2>/dev/null; then + echo "/tmp/$(basename "$f")" + return 0 + fi + sleep 2 + done + echo "pfput: upload of $f to pf0 failed" >&2 + return 1 } -pfload() { local r; r=$(pfput "$1"); pf0 "/opt/pf/pfctl -f $r" && return 0; return 1; } +pfload() { local r; r=$(pfput "$1") || return 1; pf0 "/opt/pf/pfctl -f $r" && return 0; return 1; } ok() { echo "OK $*"; } bad() { echo "NOT OK: $*" >&2; FAILED=1; } finish(){ [ "${FAILED:-0}" = 1 ] && exit 1; exit 0; }