Good catch!

Mostly LGTM too.

In D35067#797292, @emaste wrote:

Can we include the command(s) used to generate the certs somewhere, maybe a comment in the test?

• add NET_EPOCH_ASSERT
• return 'int' for errors rather than 'bool'

Change return type to int, to clarify that this is an error return.

In D35158#797201, @glebius wrote:

The new function is bool, to tell if the packet was consumed. Let's call this modern FreeBSD style. You also pass a pointer to pointer, so function can NULL-ify to express same fact of consumed mbuf. This matches classic OpenBSD/pf style. In one case you use FreeBSD style and in two cases OpenBSD.

IMHO, the new function should be used consistently using either return value or the pointer. Personally I prefer FreeBSD style over OpenBSD/pf.

• Separate counters for allocation failures
• Use array for counters for simplified init and cleanup

Improve logging

In D35144#796757, @se wrote:

I see 3 alternatives to the current approach:

1. Use a better algorithm with lower time complexity
2. Insert addresses at their correct position in ip address order (possibly also in a different data structure, e.g. a balanced tree)
3. Sort for display, keep the sorted list and set a flag (cleared when another address is added) to skip sorting if not necessary

The sorting is relatively slow since there is a complex comparison function defining the desired order and the data to be sorted is in a linked list, not an array.

I strongly suspect the sorting is slow because the sorting algorithm is incorrectly implemented, and it's doing a *lot* more work than it needs to do. I recommend investigating that first, because making the sorting faster is a lot less controversial than not sorting in certain circumstances.

Isn't there room for improving sortifaddrs() so it works well on machines with many addresses? It's not ideal to only sort some of the time, and it really shouldn't take all that long to sort 4000 elements.

In D34704#786704, @zec wrote:

This change builds on top if_index globalization (91f44749c6feb50f39af8805dd803e860f0418f1) which I strongly objected to, and which glebius agreed to back out as outlined in https://github.com/glebius/FreeBSD/commits/backout-ifindex, but that never happened. Hence, pls. don't proceed with this until if_index is reverted back to per-VNET state.

Update copyright.
Work started in 2021, so 2021-2022 is more appropriate.

Clear checksum flags from the mbuf. We don't do any checksum verification, and
the checksum flags for the outer layer IP(v6)/tcp|udp packets are no longer
meaningful.

• Don't reset the timeout callout for every packet
• Ensure softc and peer don't get deleted while crypto operations are running.

Long-lived keys

I've posted a first simple test case in D35067.

Review remarks

(In progress, I owe you a few more fixes.)

Review remarks

• review remarks

In D34340#793593, @markj wrote:

Do you have some plan to add a man page and some regression tests in a follow up?

• review remarks

• fix crash (The default rule V_pf_default_rule is initialised separately, and did not have timestamp allocated)
• Only print the date, not the timestamp for users
• If the rule was never hit show 'N/A'

I'm going to propose a tweaked version.

For those following along at home, this is getting pretty close to done, so if you want to review it now is a good time. It's probably going to land soon.

In D29554#793425, @i.dani_outlook.com wrote:

Thanks for your fast reply. It might be a bug fix, but it's also a breaking change ;-). What would you recommend to simulate this behavior as we can't use "route-to" anymore (because it doesn't create the correct state)?

In D29554#793419, @i.dani_outlook.com wrote:

Hey @kp
We've recently upgraded from FBSD 12.2 to FBSD 12.3, which contains this change. First of all: This was a breaking change for us and our rulesets didn't work anymore like before. This took us multiple hours of debugging to find the reason why. We then finally found out, that the problem is with all rules where we use route-to. Which then finally led us to this commit. After that we've found the following discussion on the mailing list: https://groups.google.com/g/bsdmailinglist/c/uMusHsnkY5s
For us it's not understandable and quite frustrating that a breaking change doesn't get announced in the relnotes or at least in the UPDATING file.

Now to the problem we have: We run pf in "block all" mode. We have an IPsec VPN (StrongSwan) that receives pakets via enc0 and pf passes them in and add's a tag (DCOUT) on the packet. Then the packet would go out on lagg0.100 but get's rerouted to lagg0.200 via a pass out rule with route-to. The route-to rule contains "flags any no state", so we do not interfere with the state table. As the last rule we have a "pass out on lagg0.200 all tagged DCOUT" which creates the correct state and passes out the paket. Due to the new behavior due to that change the last rule isn't matched anymore and the state is never setup correctly, which leds to the answer pakets being blocked by the default "block all" ruleset. Our question now is: How we can we restore the old behavior - without patching our FBSD base with reverting the change - without having to rework the whole ruleset?

Thanks and looking forward for you reply.

• fix send_ping, where we can't do a route lookup, because it's not an IP packet. Send directly to the correct peer.
• Don't bring the link down when the last peer disconnects
• fully remove peers when they time out, don't only notify userspace about it
• cope with not having keys, which can happen if userspace decides to delete them