doc: say what fragments do, and stop listing fixed items as open

todo.md had three items in both halves of itself.  Control files being
world-writable, delete 0, and a dead relay leaving half a firewall were
all in "Fixed since the review" as items 10, 11 and 12, and all three
were still sitting in "Worth fixing" underneath.  Anyone working the
open list would have redone them.  Now an item appears in one half or
the other, and the first round's entries are one line each, since the
detail is in the commit and the value of this file is the part that is
still true.

Fragments got a sentence that described the mechanism and not the
consequence.  "Later fragments now match on addresses and protocol
only" reads as though they would pass under an address rule; what
actually happens is that every rule set in fwrules(6) is written in
ports, later fragments match no port, and the datagram does not cross.
That is the right trade against reading ports out of them, which let a
crafted fragment through, but it is a hole in what works and belongs in
BUGS rather than in a changelog line.

man/fw also now says what the served /net contains, which changed
underneath it and was never written down; and it still said the card
was "not undone reliably", when nothing undoes it at all.

design.md gains the whitelist decision next to the others, because the
argument for it is the same one the rule parser already makes and the
next person to add a file to the served tree should meet it.

The mtu comment claimed a card as its reason while sitting in code both
modes use.  Between two stacks there is no card; 1500 is still right,
for a reason worth one sentence.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

1 files changed, 103 insertions, 85 deletions

diff --git a/fw/doc/todo.md b/fw/doc/todo.md
index e750674..bf4bcda 100644
--- a/fw/doc/todo.md
+++ b/fw/doc/todo.md
@@ -1,55 +1,74 @@
 # fw: open items
 
-Most of this came from a code review after the first commit. Where a
-finding has been fixed it says so and how it was checked; where it has
-not, it says what is actually true.
+Most of this came from code review. Where a finding has been fixed it
+says so and how it was checked; where it has not, it says what is
+actually true. An item never appears in both halves.
 
-## Fixed since the review
+`test/fwtest.rc` is a check for every bug that has shipped here. Run it
+twice in a row after touching anything.
+
+## Fixed, first round
+
+Verified as described in the commit; one line each here.
 
 1. **The ctl filter was a blacklist.** `headers` on a udp conversation
-   passed straight through, and the write queue is live from clone, so
-   three writes sent a datagram anywhere with no `connect` for a rule to
-   match. Now a whitelist: `bind`, `ttl`, `tos`, `ignoreadvice`,
-   `close`, `hangup`, `keepalive` pass; `connect` and `announce` are
-   rule-checked; everything else is refused by name. Verified — `headers`
-   and gre `raw` are refused, `ttl` still works, and `none.ndb` now means
-   what it says.
+   passed, and the write queue is live from clone, so three writes sent
+   a datagram anywhere. Now a whitelist of control messages.
 2. **`%M` was never installed**, so `fmtrules` emitted `ipmask=%M%` and
-   every ctl edit on a rule set containing `ip=` failed. Reproduced, then
-   fixed with `fmtinstall('M', eipfmt)`. Verified round-tripping.
+   every ctl edit on a rule set containing `ip=` failed.
 3. **`parserules` built the new list in the globals** with no lock, so
-   relays walked an empty then half-built list during any reload. Now
-   built in locals. Verified: load, reload, a rejected file leaving the
-   old rules, and edits.
-4. **A 64KB frame on a 32KB proc stack** in `etherwriteip` — the same
-   bug already fixed in `relay()` and left here. Now `Maxframe`.
-5. **`putback`/`notehandler` were dead code** — `atexit` matches on the
-   registering pid and `_exits` never runs the handlers. Removed, and
-   every doc that claimed the card was put back is corrected. Nothing
-   puts it back; see item 1 below.
-6. **Expired flows kept matching.** `flowlook` refreshed `last` without
-   checking the timeout, and reaping only happens on insert, so on an
-   idle firewall a dead flow passed traffic forever.
-7. **The pkt interface claimed a 4096 MTU** from a 1514-byte card. Now
-   `mtu 1500`. Verified: `pkt0 maxtu 1500`.
-8. **Ports were read out of non-first fragments**, so fragmented traffic
-   was denied and a crafted fragment could pass. Later fragments now
-   match on addresses and protocol only.
-9. **Four holes in the filtered `/net`**: `/net/ndb` and `/net/log` were
-   writable (mode 0666 — reconfigure name service, or trace every
-   connection on the machine), and `ipifc/*/data` was readable, which on
-   a machine also running `fw -e` is the packet stream itself. All
-   closed, reads still work. Verified.
-10. **Control files were world-writable.** Now 0600/0440 and owned by
-    the user running `fw` — testing caught that owning them as a
-    nonexistent user "fw" locked out the administrator too.
-11. **`delete 0` and `delete foo`** appended a rule reading `<nil>`. Now
-    refused.
-12. **A dead relay left half a firewall** — one direction unfiltered,
-    nothing to notice. Now `threadexitsall`.
-13. **IPv6 unicast under `-e`** was dropped in silence. It is still not
-    implemented — neighbour discovery is missing — but it now says so
-    once instead of pretending.
+   relays walked an empty then half-built list during any reload.
+4. **A 64KB frame on a 32KB proc stack** in `etherwriteip`.
+5. **`putback`/`notehandler` were dead code.** `atexit` matches on the
+   registering pid and `_exits` never runs the handlers.
+6. **Expired flows kept matching**: `flowlook` refreshed `last` without
+   checking the timeout, and reaping only happens on insert.
+7. **The pkt interface claimed a 4096 MTU** from a 1514-byte card.
+8. **Ports were read out of non-first fragments.**
+9. **`/net/ndb` and `/net/log` were writable and `ipifc/*/data`
+   readable** through the filtered `/net`.
+10. **Control files were world-writable.** Now 0600/0440, owned by
+    whoever runs `fw` — owning them as a nonexistent user "fw" locked
+    out the administrator too.
+11. **`delete 0` and `delete foo`** appended a rule reading `<nil>`.
+12. **A dead relay left half a firewall.** Now `threadexitsall`.
+13. **IPv6 unicast under `-e`** was dropped in silence. Still not
+    implemented, but it says so now.
+
+## Fixed, second round
+
+1. **The path filter was still a blacklist, and still had a hole.**
+   `/net/tcp/trans` and its siblings install kernel address
+   translations; devip gates them with `iseve()`, which through this
+   server is `fw`'s identity and not the caller's, so on a machine
+   where `fw` runs as eve there was no gate. Demonstrated, then
+   replaced: nothing is served unless it is named. `/net/log` was half
+   closed the same way — the write was refused when reading it was the
+   leak — and `ipifc/*/snoop` is the same wire as `data` and was never
+   mentioned. Sixteen new checks; six of them fail against the old
+   `netfs.c`.
+2. **The suite poisoned its own next run.** Six `fw` processes and
+   their pkt interfaces survived every run, and the next run's stack
+   routed out the dead one. It failed deterministically on the second
+   run and a comment blamed timing, which is worse than the leak: it
+   told the reader to disbelieve a real result. Now torn down by
+   unbinding the interfaces, with two checks at the end that count what
+   is left.
+3. **Checks that passed for the wrong reason.** A "refused" result was
+   returned for any failure at all, so a check for a hole passed on a
+   kernel that never had the hole. Each is now paired with one asking
+   whether the thing being refused exists. Invisibility is asked by
+   stat, not by read: reading `log` or a `data` file blocks, so the
+   read version would hang on exactly the build that still serves them.
+4. **The dead code from item 5 above was not actually deleted** — only
+   its call sites were, and the comment meant to be moved was copied,
+   so the file carried the same twelve lines twice.
+5. **Two compiler warnings** had stood since the program was written.
+   The suite now builds from clean and asks the compiler whether it had
+   anything to say.
+6. **The frame buffer is the caller's**, sized from the same `Maxpkt`
+   that sizes the read, rather than a 16KB automatic on a 32KB proc
+   stack.
 
 ## Still open
 
@@ -61,12 +80,12 @@ go, leaving the card bound to nothing and the machine with no network.
 `fw` cannot restart unaided either: the address it would read off the
 card is the address that just vanished.
 
-The cleanup that claimed to handle this was dead code and has been
-removed. The answer is not more note handling — it has to be something
-that outlives `fw`, which means the supervisor, with the address stored
-where `fw` does not own it. This also blocks `svc` supervision.
+The cleanup that claimed to handle this was dead code and is gone. The
+answer is not more note handling — it has to be something that outlives
+`fw`, which means the supervisor, with the address stored where `fw`
+does not own it. This also blocks `svc` supervision.
 
-### deny=in ip=... silently never matches in namespace mode
+### 2. deny=in ip=... silently never matches in namespace mode
 
 An `announce` sets `anyip`, and `matchrule` skips rules naming an
 address, so a peer-address inbound rule is a no-op there while working
@@ -74,6 +93,20 @@ at the packet layer. It is a reasoned decision, but by this project's
 own standard it should either be enforced at listen time (check
 `remote`, hang up) or refused at startup in that mode.
 
+### 3. A fragmented datagram does not cross
+
+Later fragments carry no transport header, so they match no port, and
+every rule set in `fwrules`(6) is written in ports. The first fragment
+crosses and the rest are denied, leaving the receiver holding an
+incomplete train until it times out. Reading ports out of them, which
+is what happened before, was worse — a crafted fragment whose payload
+bytes matched an open flow went through — so this is the right trade,
+but it is a hole in what works, not only in what is checked.
+
+Tying later fragments to the first would mean keying on
+(protocol, source, destination, id) and letting the first fragment's
+verdict stand for the rest.
+
 ## Worth fixing
 
 - **`promiscuous` injects neighbours' unicast into the protected
@@ -82,8 +115,6 @@ own standard it should either be enforced at listen time (check
   traffic, into the rules, the stats and the flow table. It is needed
   for multicast reception, so keep it and filter on the destination MAC
   in `etherin`: accept `ourmac` or `d[0]&1`.
-- **Control files are world-writable.** `ctl` and `rules` at 0666 means
-  any user can rewrite the firewall. 0660 or 0600.
 - **Use-after-free of the matched rule.** `matchrule` returns `*rp`
   after dropping `rulelock` and callers read `rule->log` outside it.
   Copy the flag out under the lock.
@@ -92,11 +123,6 @@ own standard it should either be enforced at listen time (check
   be missed.
 - **`fmtrules` truncates silently at 64K**, so a large rule set loses
   rules on every ctl edit and on save. Detect and fail.
-- **`delete 0` / `delete foo`** falls into the append branch with a nil
-  argument and appends a rule reading `<nil>`.
-- **A dead relay proc leaves a half-firewall** — the proc returns, the
-  others carry on, one direction is permanently dead with nothing to
-  notice. `threadexitsall` is the fail-closed answer.
 - **Static error buffers race under `srvrelease`** — `netfs.c` and
   `rules.c` both return `static char err[128]` while running multi-proc.
   Worst case is a wrong diagnostic, which is the thing namespace mode
@@ -108,39 +134,31 @@ own standard it should either be enforced at listen time (check
 
 - `relay()` and `permitted()` are the same decision logic twice with
   divergent log text; the gateway path should call `permitted()`.
-- The `reclaim` comment is orphaned above `putback`.
-- `putback`'s "the ctl fd must stay open" is wrong —
-  `ethermedium.unbindonclose` is 0.
-- `fswalk1` does not reject names containing `/`.
 - `fwstart`'s `mntgen` inherits the caller's fds — the trap CLAUDE.md
   documents.
-
-## Still open from before
-
-- **Broadcast handling is written but never observed.** The mapping is
-  standard and normal traffic is unaffected, but no broadcast has been
-  seen crossing `fw`.
-- **fw daemonizes**, so `svc` cannot supervise it. Needs a foreground
+- `fw` daemonizes, so `svc` cannot supervise it. Needs a foreground
   mode; `ready=srv:name` fits, since `-s` already posts to `/srv`.
   `fwstart` is the wrong shape and should probably go.
-- **One card per fw, and rules cannot name a card.** Wants repeatable
-  `-e` and an `ifc=` attribute together.
-- **Tflush is not implemented.** Only affects namespace mode. ~80 lines,
+- One card per fw, and rules cannot name a card. Wants repeatable `-e`
+  and an `ifc=` attribute together.
+- `Tflush` is not implemented. Only affects namespace mode. ~80 lines,
   with a race that cannot be fully closed.
-- **Positional `delete n` renumbers.**
-- **Never tested**: the gateway with two real VMs, any IPv6 traffic, a
-  real second NIC carrying traffic.
-
-## Docs that are now wrong
-
-- `design.md`, `todo.md` and `man/fw` all said `fw` "tries to put the
-  card back as it exits, which covers an orderly stop". It covers
-  nothing — see 5.
-- `man/fw` SYNOPSIS shows `-a` as required with `-e`; it is optional.
-- `man/fw` says an empty rule file means no network at all; see 1.
-- `design.md` presents "cs and dns come along for free" as a benefit. It
-  is also why DNS cannot be blocked in namespace mode, which belongs
-  next to it.
+- Positional `delete n` renumbers.
+
+## Never tested
+
+- **Card mode**, by the suite, on purpose: it takes the machine's card
+  away and a test that can leave you with no network is a test nobody
+  runs. A unit harness for `ether.c` — point `efd` at a pipe, call
+  `etherwriteip`, read the frame back — would cover the frame path and
+  the broadcast mapping below without touching a real card.
+- **Broadcast handling.** Written, reviewed, never observed crossing
+  `fw`. The mapping is the standard one and normal traffic is
+  unaffected.
+- **IPv6 traffic**, in any mode. Under `-e` it cannot work at all:
+  there is no neighbour discovery, so v6 unicast is dropped.
+- **The gateway with two real machines**, and a real second NIC
+  carrying real traffic.
 
 ## Deliberately not doing