Don't unwrap when removing invalid TCP state from fastpath.

[FelixMcFelix]

Now that the fastpath temporarily drops (and reacquires) the port lock when TCP state needs to be invalidated, we opened ourselves up to the possibility that another packet could have removed this state ahead of us. Equally, a packet could insert new TCP state which we might accidentally remove.

This PR removes the unwrap on removal to account for the race, and only removes TCP flows if they are pointer-equal.

Closes #618, closes #624.

[FelixMcFelix]

I sadly haven't been able to locally reproduce the crash itself, using either zone-to-zone traffic or between local VMs via standalone omicron. Both this PR and master stand up against while true; do iperf -c 10.0.0.1 -P128 -t 2; iperf -c 10.0.0.1 -P128 -t 2 -R; done for around 10 minutes. This should slam the TCP flow state table pretty hard, and exposes us to a lot of leftover entries when data segments arrive later than expected -- I would have thought this would get us a trigger, but alas.

[rcgoodfellow]

I sadly haven't been able to locally reproduce the crash itself, using either zone-to-zone traffic or between local VMs via standalone omicron. Both this PR and master stand up against while true; do iperf -c 10.0.0.1 -P128 -t 2; iperf -c 10.0.0.1 -P128 -t 2 -R; done for around 10 minutes.

Maybe try this for many flows simultaneously with distinct address pairs? This would get a bit closer to rack traffic conditions.

[FelixMcFelix]

Maybe try this for many flows simultaneously with distinct address pairs? This would get a bit closer to rack traffic conditions.

I've bumped up the topology a bit (all instances running omicron's builtin alpine linux):

┌──────────────┐    ┌──────────────────────────────────────────────────────┐
│   Macbook    │    │Farme                                                 │
│(10.0.23.168) │    │(10.0.147.187)                                        │
└──────────────┘    │  ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─   │
        ▲           │   VPC                                             │  │
        └─────SSH───┼──┼─────────┬──────────────┬──────────────┐           │
                    │            │              │              │        │  │
                    │  │         ▼              ▼              ▼           │
                    │    ┌──────────────┬──────────────┬──────────────┐ │  │
                    │  │ │     cafe     │  restaurant  │     bar      │    │
                    │    │I: 172.30.0.5 │I: 192.168.0.5│I: 172.30.0.7 │ │  │
                    │  │ │E: 10.1.222.12│E: 10.1.222.13│E: 10.1.222.14│    │
                    │    └──────────────┴──────────────┴──────────────┘ │  │
                    │  │         ▲              │              │           │
                    │            │              │              │        │  │
                    │  │         └──────────────┴──────────────┘           │
                    │                  iperf3 client->server            │  │
                    │  └ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─   │
                    └──────────────────────────────────────────────────────┘

No crashes as yet on master under the above -P128 (×4 servers on cafe) rapid-fire iperf gauntlet, but I did come across #624. I've been attempting concurrent API operations (e.g., firewall rule add/delete) in case epoch bumps tell part of the story (as per oxide-dogfood on the most recent coredump). I'll leave it running, but we're missing something to make the original fault dead reproducible.

[luqmana]

LGTM. Just left one suggestion for coalescing the flow state lookup+removal.

[luqmana]

If it's ok to swap the order here, you could make use of the Entry API to lookup and remove the tcp flow to save on searching through it twice:

Suggested change

	if let Some(found_entry) = local_lock.tcp_flows.get(ufid_out) {
	if Arc::ptr_eq(found_entry, &entry) {
	self.uft_tcp_closed(&mut local_lock, ufid_out, ufid_in);
	_ = local_lock.tcp_flows.remove(ufid_out);
	}
	}
	if let Entry::Occupied(found_entry) = local_lock.tcp_flows.entry(*ufid_out) {
	if Arc::ptr_eq(found_entry.get(), &entry) {
	_ = found_entry.remove_entry();
	self.uft_tcp_closed(&mut local_lock, ufid_out, ufid_in);
	}
	}

[FelixMcFelix]

Thanks a lot for the review Luqman.

I think this would be a good idea (the swap is valid as I read it), except local_lock.tcp_flows here is a FlowTable and not a BTreeMap. It'd be nice to forward the Entry API while respecting the capacity constraints in FlowTable::add etc. to enable that. I'll open a ticket.

EDIT: #627.