Improve the situation a bit: force logging if state creation fails.
This isn't totally right as we'll end up logging the packet twice in
this case, but it's better than not logging the drop at all.

Improve some comments.

Restore the old value of _Static_assert(sizeof(struct pf_kstate))

Plug holes in pf_rule_actions

Use style(9)-compliant boolean tests

As promised yesterday, I propose how to get further with simplifying source node handling. This is a draft/proposal, however it compiles and passes all tests, at least the ones not skipped, which for me are dummynet and altq, I need to revive my custom kernel config to get those running.

Change limited to bool. Fix one missing PF_SRC_NODE… macro.

Make pf_hashsrc() available from everywhere. Use PF_SRC_NODE… macros now that pf_hashsrc() is accessible. The OB1 error in pf_insert_src_node() is gone, adjust the tests accordingly. Simplify pf_src_connlimit() logic,

This patch only aims to fix issues with locking, not the other issues found in D39880 (which will require some modifications, as while writing this one, I found some issues with the 2 different paths reaching pf_map_addr_sn(), which D39880 does not take into account). I suppose it could be MFC'd into the currently maintained releases of FreeBSD after more testing.

Duplicate of https://cgit.freebsd.org/src/commit/?id=6463b6b59152fb1695bbe0de78f6e2675c5a765a

There's no need for additional variable, time is used only once.

This code suffers from very old OpenBSD idea of (ab)using the same data structure for in-kernel storage and communication with userspace over ioctl. There is no need for struct pf_threshold *t, as pfctl only displays its count and seconds properties. Instead of having a separate function nlattr_add_pf_threshold() with all its PF_TH_… variables we could just add new variables PF_SN_CONN_RATE_RATE and PF_SN_CONN_RATE_SECONDS directly into the source node. And rename PF_SN_CREATION to PF_SN_AGE since it's the age of the SN, calculated during the export, not its creation time, as stored in kernel.

This is also broken on FreeBSD 14.

In D47543#1085347, @franco_opnsense.org wrote:

So regardless of why I already stated this is a technical issue that is by no means "pointless", what do you suggest to improve this particular test to make it more robust? Uncontrolled creation of processes that inherit file descriptors isn't exactly clean design but I can see why you do not want to apply this mere bandaid with that larger issue at hand.

Closed by commit rG65b20771713c: pf tests: Simplify handling of pfctl -s

In D47435#1081961, @kp wrote:

Removed debug echos from tests.

A more recent, slightly simplified version.

In D46868#1068815, @kp wrote:

Completely remove pf_krule_ptr

Add test for another source, describe tests better, check for source nodes in order-independent manner.

Update rules to match only on inet6 proto tcp.

Fix wrong cleanup function. Rename the test to …_rule because there will be in future another test for global source tracking.

Update test for the hitting the max-src-conn rule so that it works both when the state is found CLOSED or when there is no state at all.

In D46798#1067230, @kp wrote:

I'm seeing this test fail quite often (though not always) because it can't find the 4204 state. So things appear to be doing what we'd expect, but there's no state. I'm not sure why, because it looks like we only check max_src_conn a state actually exists, so we ought to see it in the state table too. Sometimes that's the case, but not always.

For maximum annoyance that problem seems to go away if I attach a dtrace probe for pf_free_state(), so right now I have no idea what's causing that.
I'm not sure if we want to go dig into what's going on, or just change the test so it accepts either the state no existing or it being CLOSED as fine.

In D46797#1067141, @kp wrote:

Thanks for spotting (and fixing) this.

One small issue though: all of your recent patches seem to end up moving files. This one moves libpfctl/libpfctl/libpfctl.c to libpfctl/libpfctl.c and sys/netpfil/pf/pf_nl.* to netpfil/pf/pf_nl.*.
I'll fix these manually before I push them, but can you take a look at how you generate future ones?

It may be worth testing tools/tools/git/git-arc.sh for this. It's what I use to pick commits out of Phabricator into my local git tree and to post local git commits to Phabricator. It makes the round trip a lot less painful.

Simplify the check_tcp_3way functions, there is no need to sniff for our own ACK. Once it's sent out, we can assume the check has passed.

Removed the change in pf.c as the error was in the ruleset.

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.