Page MenuHomeFreeBSD

pf: Re-optimize state key handling
Needs ReviewPublic

Authored by markj on Tue, Aug 18, 9:33 PM.
Tags
None
Referenced Files
F167471078: D58922.id.diff
Sat, Aug 22, 12:33 AM
Unknown Object (File)
Thu, Aug 20, 12:42 PM
Unknown Object (File)
Wed, Aug 19, 8:57 AM
Unknown Object (File)
Wed, Aug 19, 8:54 AM
Unknown Object (File)
Wed, Aug 19, 7:05 AM

Details

Reviewers
kp
Summary

pf states may be looked up using one of two keys: the stack key or the
wire key. For states involving address translation, these will be
distinct; the stack key describes the addresses seen by the local
network stack, and the wire key has the translated addresses.

Historically, pf would avoid allocating separate keys if both are
identical. This changed in commit fcdb520c1b4e ("pf: nat64") to always
allocate separate state key structures. Incidentally, OpenBSD seems to
maintain the optimization, but also has an explicit reference count
embedded in state keys.

The change breaks another optimization: pf_state_key_attach() still uses
state key pointer equality to check whether the stack and wire keys are
equal, so those checks are always false after the aforementioned commit.
Thus we never skip the second key lookup, even when that's possible
(i.e., no address translation is involved).

So, for some rulesets we're consuming more memory than needed and
performing more state key lookups. The behaviour of always looking up
the stack key also happens to break some existing rulesets involving RDR
and divert-to, which is how I noticed the problem. I think those
rulesets effectively work by accident, but it seems worth restoring the
optimization regardless.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 75678
Build 72561: arc lint + arc unit

Event Timeline

markj requested review of this revision.Tue, Aug 18, 9:33 PM

It'd be good to turn those failing rules into a test case so we notice the next time I screw up.

I'm not going to be able to review this this week (holiday).

In D58922#1352217, @kp wrote:

It'd be good to turn those failing rules into a test case so we notice the next time I screw up.

I was thinking about that, but I'm not really sure it's a valid testcase. Basically, the ruleset relies on the fact that pf_state_key_attach() skips the stack key lookup, because otherwise it'd get a collision with a state created by a rdr rule. In other words, it depends on the order that pf_state_key_attach() performs lookups, so wire/stack vs. stack/wire, and that seems like an implementation detail. Or maybe I'm missing something?

There's another regression involving divert-to which isn't resolved by this patch. I'll think about adding a test case again once I get to the bottom of that. This patch seems to stand on its own so I figured I'd just submit it as-is.

I'm not going to be able to review this this week (holiday).

No problem at all.