There never was off-by-one error in pf, the error was in the ruleset which caught IPv6-specific multicast packets.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I've withdrawn the move of the check from within to the outside of pf_create_state(). The way source nodes are cleaned up in case of state creation failure in the current code would not play nice with this change. The change comes from my own branch where source nodes allocation is performed slightly differently. I'll re-upload this patch when the time is right.
Comment Actions
There's something else going on here. The original check was correct.
With this patch I see this during the test case:
pass in on epair0b all flags S/SA keep state (max 3) [ Evaluations: 9 Packets: 5 Bytes: 384 States: 4 ] [ Inserted: uid 0 pid 0 State Creations: 4 ]
and
all ipv6-icmp ff02::16[143] <- fe80::b9:a0ff:fef2:fe0a NO_TRAFFIC:NO_TRAFFIC age 00:00:03, expires in 00:00:07, 2:0 pkts, 156:0 bytes, rule 3 all ipv6-icmp fe80::c5:5dff:fe25:120a -> ff02::16[143] NO_TRAFFIC:NO_TRAFFIC age 00:00:03, expires in 00:00:17, 1:0 pkts, 88:0 bytes, rule 4 all ipv6-icmp 2001:db8:42::1[135] <- 2001:db8:42::2 NO_TRAFFIC:NO_TRAFFIC age 00:00:02, expires in 00:00:10, 3:3 pkts, 216:216 bytes, rule 1 all tcp 2001:db8:43::2[666] <- 2001:db8:42::2[4201] CLOSED:SYN_SENT [0 + 1] [0 + 2] age 00:00:02, expires in 00:09:58, 1:0 pkts, 76:0 bytes, rule 3 all tcp 2001:db8:42::2[4201] -> 2001:db8:43::2[666] SYN_SENT:CLOSED [0 + 2] [0 + 1] age 00:00:02, expires in 00:09:58, 1:0 pkts, 76:0 bytes, rule 4 all tcp 2001:db8:43::2[666] <- 2001:db8:42::2[4202] CLOSED:SYN_SENT [0 + 1] [0 + 2] age 00:00:01, expires in 00:09:59, 1:0 pkts, 76:0 bytes, rule 3 all tcp 2001:db8:42::2[4202] -> 2001:db8:43::2[666] SYN_SENT:CLOSED [0 + 2] [0 + 1] age 00:00:01, expires in 00:09:59, 1:0 pkts, 76:0 bytes, rule 4 all tcp 2001:db8:43::2[666] <- 2001:db8:42::2[4203] CLOSED:SYN_SENT [0 + 1] [0 + 2] age 00:00:00, expires in 00:10:00, 1:0 pkts, 76:0 bytes, rule 3 all tcp 2001:db8:42::2[4203] -> 2001:db8:43::2[666] SYN_SENT:CLOSED [0 + 2] [0 + 1] age 00:00:00, expires in 00:10:00, 1:0 pkts, 76:0 bytes, rule 4
So we do in fact have 3 states, but r->states_cur is 4. I think that's the actual bug. Something is causing us to mis-count, and that's why we had the original off-by-one.
| netpfil/pf/pf.c | ||
|---|---|---|
| 5335 ↗ | (On Diff #143745) | There's a separate check for this in if_pfsync.c too. I don't think we need to change either one (see my other comment), but we certainly need to keep those in sync. |
| sys/netpfil/pf/max_states.sh | ||
|---|---|---|
| 43 ↗ | (On Diff #143745) | *headdesk* It took me way too long to figure this out. This rule is our problem. We're matching traffic other than the TCP packets we're looking for. If we look at the state table we see this: all ipv6-icmp ff02::16[143] <- fe80::b9:a0ff:fef2:fe0a NO_TRAFFIC:NO_TRAFFIC age 00:00:03, expires in 00:00:07, 2:0 pkts, 156:0 bytes, rule 3 That matched the rule with (max 3), and ICMP6 type 143 is an MLD Listener Report, which makes sense for a node to be sending. So there is no bug. Or not in pf anyway, it's the test that isn't quite right. If we change the rule here to add proto tcp the test will pass without the pf code change. |
Comment Actions
Removed the change in pf.c as the error was in the ruleset.
| sys/netpfil/pf/max_states.sh | ||
|---|---|---|
| 43 ↗ | (On Diff #143745) |
I suppose this should be me headdesking. A very good catch! |