I reckon queue and rtable should be marked up here

this should be: "the logging is done"

this should probably be understood as a warning

Can we keep state on match rules?

This line is redundant. 6248 already has match. (Also, this list needs to be sorted alphabetically, so it's in the wrong place too).

We've got this twice. See also line 1127.

M_NOWAIT can fail allocation. We need to deal with the ri == NULL case here.

if (r->log && pd->act.log) ?
Also, given the pf_rule_to_actions() above, is it even possible for pd->act.log to be true and r->log be false? Or vice versa?

V_pf_status.reass = *reass & PF_REASS_ENABLED?
If nothing else that prevents us from accidentally setting random flags briefly, which might turn into a very difficult to debug race condition years down the line.

Does this mean we scrub even when there's no scrub rule?

I don't think we should. As far as I understand OpenBSD the "match" rules only allow for setting multiple parameters (like scrub or nat) without specifying if packet is passed or blocked. Match condition A? Then set mss. Match condition B? Then log. Finally match condition C, "pass" and create a state, thus applying actions from "match" rules. If "match" rules created states how would they be different from "pass" rules?

I've checked how this works on FreeBSD 13.1 GENERIC (since it has some support of pass rules) and it seems broken. You can specify "match … keep state". The syntax is accepted but it does not result in creation of a state.

Older FreeBSD code would ensure that "keep state" is possible only for "not-block" and the only "not-block" would be "pass". But since ef950daa35d43dd396958ca28ce9de0514daf873 there is also "pass". Which can't create state. But the syntax allows for it.

Maybe this particular line of the my patch could be even be ported to the existing FreeBSD 13?

Fixed.

@kp I'm unsure if I did the math correctly here. Now I've updated it to 336B, because its dividable by 64b/8B and 340B * 12 = 4032, so it fits within a page. Is this proper?

Fixed.

Indeed that was wrong. With logging of "match" rules directly when matching happens, and the same for the NAT rule, there is no need for those complicated checks. In fact this is what OpenBSD did too later. I've updated the code to match that of OpenBSD right before introduction of PF_LOG_MATCHES.

That would rather be

V_pf_status.reass = *reass & (PF_REASS_ENABLED|PF_REASS_NODF);

I think the issue is that there is no good definition of what scrubbing actually is. This single name carries out two very distinctive operations:

1. Sanitation of IPv4¹, IPv6¹ and TCP².
2. Applying scrub modifications to the IP and TCP layers³.

As far as I understand in FreBSD sanitation is optional. Once say "no scrub" (or even if you have no scrub rules? Is there a default scrub rule?) you forfeit all benefits of sanitation. Even if your goal was just to, for example, not have MSS clamping for particular traffic.

OpenBSD's approach is that general sanitation is unavoidable and fragmentation handling is controlled by the set reassemble yes | no [no-df] option in pf.conf.

The way I wrote this patch is that if "scrub" rules are present they are obeyed for sanitation of packets. If there are none, sanitation happens always, like in OpenBSD. But now I see that I had forgotten to make such override for TCP. I'll update the patch alter to address this issue.

[1] Length in header not matching the measured length, too small header length, fragment flag without fragment offset, badly-aligned fragment offset, too big total length of fragmented packets, lack of jumbo flag for big IPv6 packets, broken IPv6 options.

[2] Improper TCP flags, bad header length.

[3] Enforce MTU / Hop Limit, enforce TCP MSS.

At this moment pfync and pfctl are not yet fixed to handle the 16-bit flags. I'll update them later.

This conflicts with PFRULE_DN_IS_PIPE which is also assigned to pf_rule_actions->flags.

I don't think we should. As far as I understand OpenBSD the "match" rules only allow for setting multiple parameters (like scrub or nat) without specifying if packet is passed or blocked. Match condition A? Then set mss. Match condition B? Then log. Finally match condition C, "pass" and create a state, thus applying actions from "match" rules. If "match" rules created states how would they be different from "pass" rules?

Ah, I see.

Maybe this particular line of the my patch could be even be ported to the existing FreeBSD 13?

Possibly. I try to avoid behaviour changes on stable branches, but perhaps this is a bug fix more than a behaviour change.

This seems to be why the pfctl pf0039 test fails. We no longer print 'fragment' on the rule that had it set.

We're going to have to be very careful about pfsync compatibility then.

That should be okay, yes. The intent behind this (I believe, I didn't write this) is to keep struct pf_kstate compact for performance reasons. It's almost inevitable that we're going to keep growing it with new features though.

Fixed.

I've done it the same way as OpenBSD did during transition. There are 2 spare bytes, I've added a new struct member there. In sending and receiving both the the old 8-bit and the new 16-bit struct members are used.

@kp I think the simplest way to address this conflict (as I'd rather keep PFSTATE_.* flags with the same values as OpenBSD does) is to provide pf_rule_actions->dnflags and set it in pf_rule_to_actions(). This is is what I have uploaded now.

However it makes me raise a question why are there are now 2 separate variables for those flags (pfctl_rule/pf_krule->free_flags and pfctl_eth_rule/pf_keth_rule->dnflags). And there still is the 32-bit pfctl_rule/pf_krule->rule_flags which could store those flags just fine if they are moved to higher bits.

It's 'dnflags' for ethernet rules, 'free_flags' for layer-3 rules.

We can't move the flags, because that'll break ABI compatibility with userspace.

Please find the updated patch where DN flags follow the existing mechanism of translating PFRULE_.* flags on rules into PFSTATE_.* flags on states.

With flags translated onto pf_kstate->state_flags (s->state_flags |= pd->act.flags in pf_create_state()) in we can hope to be able to make those flags work properly with pfsync too one day. That is out of scope of this review and I'll submit a proposal for pfsync separately soon-ish but let's already have code preparing us for that opportunity.

Sounds like a good plan.

I expect to land this (and the tests) late next week or early the week after, once I'm back from AsiaBSDCon.

I've borrowed this idea of combining the 8- and 16-bit state flags from OpenBSD before I came up with D39392. If D39392 gets approved, this part could be removed.