# fw: open items

Most of this came from code review. Where a finding has been fixed it
says so; where it has not, it says what is actually true. An item never
appears in both halves.

`test/fwtest.rc` is a check for every bug that has shipped here — 77 of
them now. Run it twice in a row after touching anything.

## Fixed, first round

1. **The ctl filter was a blacklist.** `headers` on a udp conversation
   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.
3. **`parserules` built the new list in the globals** with no lock, so
   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.**
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.

## Fixed, second round

1. **The path filter was still a blacklist, and still had a hole.**
   `/net/*/trans` installs kernel address translations, gated by
   `iseve()` — which through this server is `fw`'s identity, not the
   caller's. Replaced with a list of what is served. `/net/log` was
   half closed the same way, and `ipifc/*/snoop` is the same wire as
   `data`.
2. **The suite poisoned its own next run**, and a comment blamed
   timing. Six `fw` processes and their interfaces survived every run.
3. **Checks that passed for the wrong reason**: "refused" was returned
   for any failure, so a check for a hole passed on a kernel that never
   had the hole.
4. **The dead code from item 5 above was not actually deleted.**
5. **Two compiler warnings** had stood since the program was written.
6. **The frame buffer belongs to the caller**, not to a proc stack.

## Fixed, third round

1. **matchrule returned things whose lifetime it did not own** — a
   `Rule*` and a static error buffer, both read after it released the
   lock, so a rule set installed in between freed one and two procs
   deciding at once overwrote the other. Now a `Match` in the caller's
   frame. `netfs.c` had its own static buffer with the same problem.
2. **revalidate counted as traffic.** Re-checking live flows after an
   edit charged each one to the rule that matched, so `stats` answered
   a different question after every edit.
3. **A rule set was 64 kilobytes long.** `fmtrules` clipped, and since
   every ctl edit formats the set out and parses it back, editing a set
   past the limit truncated the *rules*: 2000 in, 1214 out, and the
   next edit failed. `flows` had the same cap.
4. **A fragmented datagram did not cross.** Later fragments matched no
   port and every rule set is written in ports: measured, one fragment
   passed and two dropped. The first fragment now decides and the train
   inherits.
5. **`deny=in ip=...` was a no-op in namespace mode.** An announce
   names no peer, so rules naming one were skipped and nothing asked
   again. The peer is now checked at listen time, and a refused caller
   is hung up rather than handed to the program.
6. **`fwstart` had never worked.** `mntgen /mnt/fw` was run only when
   `/mnt/fw` was missing, which is the one case where mount(2) fails,
   so the control directories never appeared. It also left its servers
   holding the console.
7. **The log named an address without its port** — `getfields` writes
   over the separators it splits on.
8. **relay() and permitted() were the same logic twice**, already
   drifted apart in their debug output.
9. **The interface tables were read into fixed buffers** and searched
   as blobs rather than parsed. Not the defect it was reported as — the
   default route sorts first, so it was always found — but the device
   name was matched against the whole status text, addresses included.
10. **Promiscuous mode had no filter behind it.** The card must be
    promiscuous for multicast, but every neighbour's unicast was then
    judged, counted and flow-tracked as if it were ours.
11. **"fw daemonizes, so svc cannot supervise it" was wrong.** `svc`
    has had the detaching shape from the start: `ready=srv:` watches
    the `/srv` name, and for those services the process exiting "is
    normal and never causes a restart". Nothing needed a foreground
    mode. What was true underneath it: the control filesystem going
    away ended the server proc and left the relays filtering, so the
    `/srv` name could be gone while the firewall was still running.
    Now any part stopping stops all of them, which is what makes the
    name worth watching. `fwstart` is still the wrong shape; a service
    file per card is in `lib/svc`.

## Still open

### 1. Nothing puts the card back

Taking a card is destructive and is not undone. The pkt interface is
`unbindonclose`, so when `fw` stops the interface and the address both
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 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 is what makes `restart=always` a half-measure rather than an
answer: `svc` will start `fw` again, and the new one has no address to
read off a card that no longer has one. It needs `-a`, or the card
configured again first. Supervision works; recovery does not.

### 2. One card per fw, and rules cannot name a card

Wants repeatable `-e` and an `ifc=` attribute, together.

### 3. Tflush is not implemented

A request `fw` is blocked on cannot be abandoned. Only affects
namespace mode, where waiting for an inbound connection is the one
operation that blocks indefinitely. ~80 lines, with a race that cannot
be fully closed: between the syscall returning and the handler clearing
its entry, a note may land on a worker that has moved on.

### 4. Positional `delete n` renumbers

Inherent to positional deletion; iptables has it too. Said out loud
here rather than fixed.

## Never tested

- **The wire side of card mode.** The suite can take a spare card — it
  does, and everything up to the wire is now covered — but it cannot
  make a neighbour send to it. Anything that depends on another machine
  on the same segment is unproven: the destination-address filter, ARP
  against a real peer, broadcast.
- **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. IPv6
  extension headers are not walked, so v6 fragments do not cross.
- **The gateway with two real machines**, and a real second NIC
  carrying real traffic.

## Deliberately not doing

**NAT**, rate limiting, fragment reassembly, ICMP type matching, deep
IPv6. Scope, not difficulty.