Fix source tracking for route-to rules and for global tracking

[vegeta_tuxpowered.net]

Summary

For every state pf creates up to two source nodes: a limiting one struct pf_kstate -> src_node and a NAT one struct pf_kstate -> nat_src_node. The limiting source node is tracking information needed for limits using max-src-states and max-src-nodes and the NAT source node is tracking NAT rules only.

On closer inspection three issues emerge:

• For route-to rules the redirection decision is stored in the limiting source node. Thus sticky-address and source limiting can't be used separately.
• Global source tracking, as promised in the man page, is totally absent from the code. Pfctl is capable of setting flags PFRULE_SRCTRACK (enable source tracking) and PFRULE_RULESRCTRACK (make source tracking per rule). The kernel code checks PFRULE_SRCTRACK but ignores PFRULE_RULESRCTRACK. That would make all source tracking always global but the code is written in a way that makes source tracking work per-rule only.
• Once OpenBSD syntax is imported, we might need even more SNs per rule. It should be possible to have limits, nat and route-to decisions in a single rule.

This patch is based on OpenBSD approach where source nodes have a type and each state has a list of source nodes instead of just two pointers.

The patch also fixes multiple regarding source nodes:

• Source nodes are not created anymore with zeroed address to be filled in later. The lock is held throughout the whole process of SN search, creation and LB decision.
• Existence of SNs created during ruleset evaluation is checked when creating a state, in case the SNs have been killed.
• Killing specified SNs has been merged with killing all SNs, which also solves locking issues in pf_clear_srcnodes().
• A minimal struct pf_test_ctx has been created, to be used more in the future, like in OpenBSD.
• The approach to make global tracking work is to attach global source nodes to the default rule. This functionality seems broken in OpenBSD too. I've checked their code history, there was a global flag to pf_insert_src_node() at some point but it was removed.
• Off-by-one errors for limits.

[kp]

It may be worth separating out the locking improvements into standalone commits. Those sound like they're worth having independent of this work, and it'd make this a smaller, easier to digest commit.

I'm not sufficiently familiar with the src tracking code to give much meaningful advice on that.

[vegeta_tuxpowered.net]

A more recent, slightly simplified version.

[vegeta_tuxpowered.net]

Removed debug echos from tests.

[kp]

I need to do a more thorough review, but these minor points jumped out at me.
I'm going to be traveling for a few days for OpenFest, so I may not get around to it until after I'm back. Please chase me if I haven't returned to this before November 8th.

sys/net/pfvar.h

1592

I wonder if we should immediately add struct pf_pdesc here, or wait until we get to the point of importing the pf_test_ctx introduction from OpenBSD.

sys/netpfil/pf/pf.c

882

return (ret);

977–979

I think I'd prefer 'bool', or 'int'. There's really no reason for it to be anything else.

sys/netpfil/pf/pf_ioctl.c

1862

Maybe just for (enum pf_sn_types sn_type = 0; sn_type < PF_SN_MAX; sn_type++) here?

tests/sys/netpfil/pf/utils.subr

305 ↗

(On Diff #145614)

Would it be worth extracting this into its own patch?

[vegeta_tuxpowered.net]

sys/net/pfvar.h

1592

I wonder if we should immediately add struct pf_pdesc here

Let's not mix it with the currently reviewed patch. It's already big and messy enough.

I'm not 100% sure if that particular variable makes sense to be moved inside of struct pf_test_ctx - passing both pf_pdesc and pf_test_ctx does not sound that bad and allows for clear distinction what goes where. In OpenBSD pf_test_ctxhas some mixed uses too, it is used for example to have the rule evaluation set some variables (by pointer to pointer) on stack of pf_test(). Storing them within struct pf_pdesc might produce simpler code than the way OpenBSD does it.

I'll investigate it and get back to you.

sys/netpfil/pf/pf.c

977–979

Good idea.

sys/netpfil/pf/pf_ioctl.c

1862

Sure, I'll do it in all places where such loop is called.

tests/sys/netpfil/pf/utils.subr

305 ↗

(On Diff #145614)

Sure, I've created D47435

[vegeta_tuxpowered.net]

sys/netpfil/pf/pf_ioctl.c

5907

I've cherry-picked this change into D47440 as it fixes a bug in the current pf.