pf: Add support for multiple source node types

[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 some 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 makes source tracking work per-rule only.

This patch is based on OpenBSD approach where source nodes have a type and each
state has an array of source node pointers indexed by source node type
instead of just two pointers. The conditions for limiting are applied
only to source nodes of PF_SN_LIMIT type. For global limit tracking
source nodes are attached to the default rule.

Sponsored by: InnoGames GmbH

[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

1609

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

889

return (ret);

989

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

1858–1859

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

1609

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

989

Good idea.

sys/netpfil/pf/pf_ioctl.c

1858–1859

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

5975–5976

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

[vegeta_tuxpowered.net]

Source node locking issues have been solved in another review. This patch now covers only adding source node types.

[vegeta_tuxpowered.net]

sys/net/pfvar.h

1609

pf_test_ctx is gone because of previous changes which make source nodes not shared between rule parsing and state creation.

[kp]

Looks pretty close to ready to land. Just a few small bits of polishing left.

lib/libpfctl/libpfctl.h

363

I'm a little bit concerned about this, in terms of backwards compatibility. There are (or should be, anyway) a number of ports users of libpfctl, and while they don't use the in-tree libpfctl but a libpfct port instead the port is based on the in-tree libpfctl, so this change could potentially break those users.

On the other hand, if we're not going to MFC this it's probably fine. There are still relatively few libpfctl users out there. There will be more once the netlink migration is complete and I remove the ioctl interface entirely.

On the third hand, the kernel does still export sync_flags so it's not a lot of work to keep it.

sys/netpfil/pf/pf.c

2674–2675

Style nit: for (enum pf_sn_types sn_type = 0; sn_type < PF_SN_MAX; sn_type++) {

It may also be good to have a typedef enum pf_sn_types pf_sn_types_t so we can just for pf_sn_types_t ....

6014

If I'm reading things correctly we hash just the source address (and address family), so no.
That should get simplified down to a single hash.

[vegeta_tuxpowered.net]

Restored original variable names in libpfctl, added typedef for the enum.

[vegeta_tuxpowered.net]

lib/libpfctl/libpfctl.h

363

I've restored all original variables.

sys/netpfil/pf/pf.c

6014

The source node for NAT/RDR uses sk->addr[pd->sidx], not pd->src, though.

[kp]

Please do fix that one comment, but other than that approved to commit.

sys/netpfil/pf/pf.c

6014

Ah, yes, you're right. I didn't look quite far enough.

The PF_SN_LIMIT and PF_SN_ROUTE hashes would be the same (because src == pd->src), but the PF_SN_NAT one uses sk->addr, and can be different.

So the comment should either be removed or replaced by one explaining this (ideally the latter, because I was wrong about this before, and I'll be wrong about this again after I forget this discussion).