Page MenuHomeFreeBSD

WIP: pf: Fix table counters
Needs ReviewPublic

Authored by vegeta_tuxpowered.net on Tue, Oct 1, 8:49 PM.

Details

Reviewers
kp
Summary

This patch is work in progress but I'd like to ask for comments and opinions

While working on source nodes I was attracted to pd->nat_rule used in pf_route() when packets can't be send due to too small MTU and in pf_counters_inc(). The problem is that pd->nat_rule is set only during ruleset evaluation, that is only for the first packet. I thought of fixing it, this got me into the insides of pf_counters_inc() and as you can imagine more bugs have been found:

  • Double rule counter increase for match rules for the 1st packet.
  • Table counters not updated for anchors, match and nat rules.
  • Since pf_test_rule() is now called from pf_setup_pdesc() that means that the effort of setting pd and evaluation the ruleset for fragments happens before packets are checked for PF_MTAG_FLAG_DUMMYNET, PF_MTAG_FLAG_ROUTE_TO and MTAG_PF_DIVERT, which tags should have pf immediately stop processing the packets.

My idea to fix the counters is to use the match_rules variable for all type of rules, including anchor, nat and pass, instead only match rules. The variable is initiated in pf_test() at the very beginning, it collects all types of rules from pf_get_translation() and pf_test_rule(), then finally passed to pf_counters_inc() where all rules get their counters updated. There's a test attached to the patch, result of such test before the patch:

=== NAT rules ===
@0 nat on epair1a inet6 from <nat_sources:1> to any -> { 2001:db8:43::1, fe80::35:61ff:fe86:1a0a } round-robin
  [ Evaluations: 5         Packets: 3         Bytes: 212         States: 1     ]
  [ Inserted: uid 0 pid 0 State Creations: 1     ]
  [ Last Active Time: N/A ]
=== pass_rules ===
@0 block drop all
  [ Evaluations: 9         Packets: 5         Bytes: 420         States: 0     ]
  [ Inserted: uid 0 pid 0 State Creations: 0     ]
  [ Last Active Time: Tue Oct  1 17:02:19 2024 ]
@1 pass inet6 proto ipv6-icmp all icmp6-type neighbrsol keep state
  [ Evaluations: 9         Packets: 4         Bytes: 288         States: 2     ]
  [ Inserted: uid 0 pid 0 State Creations: 2     ]
  [ Last Active Time: Tue Oct  1 17:02:20 2024 ]
@2 pass inet6 proto ipv6-icmp all icmp6-type neighbradv keep state
  [ Evaluations: 7         Packets: 0         Bytes: 0           States: 0     ]
  [ Inserted: uid 0 pid 0 State Creations: 0     ]
  [ Last Active Time: N/A ]
@3 anchor "anchor1" in on epair0b inet6 proto tcp from <anchor_sources:1> to any {
  [ Evaluations: 9         Packets: 3         Bytes: 212         States: 1     ]
  [ Inserted: uid 0 pid 0 State Creations: 1     ]
  [ Last Active Time: N/A ]
@0 match from <match_sources:1> to any
  [ Evaluations: 1         Packets: 4         Bytes: 288         States: 1     ]  <- Initial SYN counted twice
  [ Inserted: uid 0 pid 0 State Creations: 1     ]
  [ Last Active Time: N/A ]
@1 pass from <state_sources:1> to any flags S/SA keep state
  [ Evaluations: 1         Packets: 3         Bytes: 212         States: 1     ]
  [ Inserted: uid 0 pid 0 State Creations: 1     ]
  [ Last Active Time: Tue Oct  1 17:02:20 2024 ]
}
@4 pass out on epair1a inet6 proto tcp all flags S/SA keep state
  [ Evaluations: 8         Packets: 3         Bytes: 212         States: 1     ]
  [ Inserted: uid 0 pid 0 State Creations: 1     ]
  [ Last Active Time: Tue Oct  1 17:02:20 2024 ]
=== tables ===
--a-r-C	anchor_sources
	Addresses:   1
	Cleared:     Tue Oct  1 17:02:19 2024
	References:  [ Anchors: 0                  Rules: 1                  ]
	Evaluations: [ NoMatch: 0                  Match: 1                  ]
	In/Block:    [ Packets: 0                  Bytes: 0                  ]
	In/Pass:     [ Packets: 0                  Bytes: 0                  ] <- Counters not updated at all
	In/XPass:    [ Packets: 0                  Bytes: 0                  ]
	Out/Block:   [ Packets: 0                  Bytes: 0                  ]
	Out/Pass:    [ Packets: 0                  Bytes: 0                  ]
	Out/XPass:   [ Packets: 0                  Bytes: 0                  ]
--a--hC	match_sources
	Addresses:   1
	Cleared:     Tue Oct  1 17:02:19 2024
	References:  [ Anchors: 1                  Rules: 0                  ]
	Evaluations: [ NoMatch: 0                  Match: 1                  ]
	In/Block:    [ Packets: 0                  Bytes: 0                  ]
	In/Pass:     [ Packets: 0                  Bytes: 0                  ] <- Counters not updated at all
	In/XPass:    [ Packets: 0                  Bytes: 0                  ]
	Out/Block:   [ Packets: 0                  Bytes: 0                  ]
	Out/Pass:    [ Packets: 0                  Bytes: 0                  ]
	Out/XPass:   [ Packets: 0                  Bytes: 0                  ]
--a-r-C	nat_sources
	Addresses:   1
	Cleared:     Tue Oct  1 17:02:19 2024
	References:  [ Anchors: 0                  Rules: 1                  ]
	Evaluations: [ NoMatch: 3                  Match: 1                  ]
	In/Block:    [ Packets: 0                  Bytes: 0                  ]
	In/Pass:     [ Packets: 0                  Bytes: 0                  ] <- Counters not updated at all
	In/XPass:    [ Packets: 0                  Bytes: 0                  ]
	Out/Block:   [ Packets: 0                  Bytes: 0                  ]
	Out/Pass:    [ Packets: 0                  Bytes: 0                  ]
	Out/XPass:   [ Packets: 0                  Bytes: 0                  ]
--a--hC	state_sources
	Addresses:   1
	Cleared:     Tue Oct  1 17:02:19 2024
	References:  [ Anchors: 1                  Rules: 0                  ]
	Evaluations: [ NoMatch: 0                  Match: 1                  ]
	In/Block:    [ Packets: 0                  Bytes: 0                  ]
	In/Pass:     [ Packets: 2                  Bytes: 136                ]
	In/XPass:    [ Packets: 0                  Bytes: 0                  ]
	Out/Block:   [ Packets: 0                  Bytes: 0                  ]
	Out/Pass:    [ Packets: 1                  Bytes: 76                 ]
	Out/XPass:   [ Packets: 0                  Bytes: 0                  ]
=== NAT rules ===
@0 nat on epair1a inet6 from <nat_sources:1> to any -> { 2001:db8:43::1, fe80::1f:dff:fee2:420a } round-robin
  [ Evaluations: 4         Packets: 3         Bytes: 212         States: 2     ]
  [ Inserted: uid 0 pid 0 State Creations: 2     ]
  [ Last Active Time: Tue Oct  1 20:25:22 2024 ]
=== pass_rules ===
@0 block drop all
  [ Evaluations: 7         Packets: 13        Bytes: 1016        States: 4     ]
  [ Inserted: uid 0 pid 0 State Creations: 4     ]
  [ Last Active Time: Tue Oct  1 20:25:22 2024 ]
@1 pass inet6 proto ipv6-icmp all icmp6-type neighbrsol keep state
  [ Evaluations: 7         Packets: 4         Bytes: 288         States: 4     ]
  [ Inserted: uid 0 pid 0 State Creations: 4     ]
  [ Last Active Time: Tue Oct  1 20:25:22 2024 ]
@2 pass inet6 proto ipv6-icmp all icmp6-type neighbradv keep state
  [ Evaluations: 5         Packets: 0         Bytes: 0           States: 0     ]
  [ Inserted: uid 0 pid 0 State Creations: 0     ]
  [ Last Active Time: N/A ]
@3 anchor "anchor1" in on epair0b inet6 proto tcp from <anchor_sources:1> to any {
  [ Evaluations: 7         Packets: 3         Bytes: 212         States: 2     ]
  [ Inserted: uid 0 pid 0 State Creations: 2     ]
  [ Last Active Time: Tue Oct  1 20:25:22 2024 ]
@0 match from <match_sources:1> to any
  [ Evaluations: 1         Packets: 3         Bytes: 212         States: 1     ]
  [ Inserted: uid 0 pid 0 State Creations: 1     ]
  [ Last Active Time: Tue Oct  1 20:25:22 2024 ]
@1 pass from <state_sources:1> to any flags S/SA keep state
  [ Evaluations: 1         Packets: 3         Bytes: 212         States: 2     ]
  [ Inserted: uid 0 pid 0 State Creations: 2     ]
  [ Last Active Time: Tue Oct  1 20:25:22 2024 ]
}
@4 pass out on epair1a inet6 proto tcp all flags S/SA keep state
  [ Evaluations: 5         Packets: 3         Bytes: 212         States: 2     ]
  [ Inserted: uid 0 pid 0 State Creations: 2     ]
  [ Last Active Time: Tue Oct  1 20:25:22 2024 ]
=== tables ===
--a-r-C	anchor_sources
	Addresses:   1
	Cleared:     Tue Oct  1 20:25:21 2024
	References:  [ Anchors: 0                  Rules: 1                  ]
	Evaluations: [ NoMatch: 0                  Match: 1                  ]
	In/Block:    [ Packets: 0                  Bytes: 0                  ]
	In/Pass:     [ Packets: 2                  Bytes: 136                ]
	In/XPass:    [ Packets: 0                  Bytes: 0                  ]
	Out/Block:   [ Packets: 0                  Bytes: 0                  ]
	Out/Pass:    [ Packets: 1                  Bytes: 76                 ]
	Out/XPass:   [ Packets: 0                  Bytes: 0                  ]
--a--hC	match_sources
	Addresses:   1
	Cleared:     Tue Oct  1 20:25:21 2024
	References:  [ Anchors: 1                  Rules: 0                  ]
	Evaluations: [ NoMatch: 0                  Match: 1                  ]
	In/Block:    [ Packets: 2                  Bytes: 136                ]
	In/Pass:     [ Packets: 0                  Bytes: 0                  ]
	In/XPass:    [ Packets: 0                  Bytes: 0                  ]
	Out/Block:   [ Packets: 1                  Bytes: 76                 ]
	Out/Pass:    [ Packets: 0                  Bytes: 0                  ]
	Out/XPass:   [ Packets: 0                  Bytes: 0                  ]
--a-r-C	nat_sources
	Addresses:   1
	Cleared:     Tue Oct  1 20:25:21 2024
	References:  [ Anchors: 0                  Rules: 1                  ]
	Evaluations: [ NoMatch: 2                  Match: 1                  ]
	In/Block:    [ Packets: 0                  Bytes: 0                  ]
	In/Pass:     [ Packets: 0                  Bytes: 0                  ]
	In/XPass:    [ Packets: 1                  Bytes: 76                 ] <- Packets counted with NATed address?
	Out/Block:   [ Packets: 0                  Bytes: 0                  ]
	Out/Pass:    [ Packets: 0                  Bytes: 0                  ]
	Out/XPass:   [ Packets: 2                  Bytes: 136                ]
--a--hC	state_sources
	Addresses:   1
	Cleared:     Tue Oct  1 20:25:21 2024
	References:  [ Anchors: 1                  Rules: 0                  ]
	Evaluations: [ NoMatch: 0                  Match: 1                  ]
	In/Block:    [ Packets: 0                  Bytes: 0                  ]
	In/Pass:     [ Packets: 2                  Bytes: 136                ]
	In/XPass:    [ Packets: 0                  Bytes: 0                  ]
	Out/Block:   [ Packets: 0                  Bytes: 0                  ]
	Out/Pass:    [ Packets: 1                  Bytes: 76                 ]
	Out/XPass:   [ Packets: 0                  Bytes: 0                  ]

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Interesting observations.

My first impulse is to say let's hold off on this for a while, because there's a lot more refactoring coming as we import more OpenBSD changes.

It would be worthwhile to add test cases for the known issues though, that way we won't forget to fix this later.