Page MenuHomeFreeBSD

Draft: pf: Move route-to information to pf_rule_actions
ClosedPublic

Authored by vegeta_tuxpowered.net on Thu, Nov 28, 3:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 8, 6:45 PM
Unknown Object (File)
Tue, Dec 3, 1:20 PM
Unknown Object (File)
Mon, Dec 2, 8:58 AM
Unknown Object (File)
Sat, Nov 30, 10:13 PM
Unknown Object (File)
Sat, Nov 30, 10:11 PM
Unknown Object (File)
Sat, Nov 30, 10:10 PM
Unknown Object (File)
Sat, Nov 30, 10:10 PM
Unknown Object (File)
Fri, Nov 29, 6:14 PM

Details

Summary

Route-to redirection information (rt, rt_kif, rt_addr) can be considered an
action of a rule. This information is duplicated in struct pf_kstate which
means that the pf_route() function must always figure out where to get this
information from: state for stateful forwarding, or rule for stateless.

Create the necessary members in struct pf_rule_action. Fill them in right after
parsing the ruleset, similar for how NAT redirection is applied right after
parsing the NAT ruleset. Remove the logic for finding the right source for
route-to redirection from pf_route().

As a bonus simplify pf_map_addr_sn() and source node handling. Both for the
NAT and the filter ruleset there is now only one path:

  1. parse the rules
  2. apply redirection either from an existing source node or by load balancing for the last matching rule
  3. create the source node using the redirection if the node does not yet exist

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

vegeta_tuxpowered.net retitled this revision from pf: Move route-to information to pf_rule_actions to Draft: pf: Move route-to information to pf_rule_actions.

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.

I need to have a closer look again tomorrow, but this looks like a good idea.
It expects to be applied on top of D47770, right?

sys/net/pfvar.h
664

uint8_t, not u_int8_t.

1113

It's not obvious to me why we're expecting pf_kstate to grow. All we're doing is moving fields around. (And indeed, we much prefer it not to grow, because that improves cache behaviour.)

Perhaps the different order causes more padding in the struct? In that case we may want to look at re-ordering a bit more to get that padding space back.

sys/net/pfvar.h
1113

Both pf_kstate and pf_rule_actions are full of holes, maybe it became even worse with them. Let me see if I can repack it.

Approved (without the 'draft' in the commit header line, of course).

sys/net/pfvar.h
1115

That's a nice improvement, but I'm not sure we should modify the assertion.
We're bound to want to add something else to the state sooner or later, and the point of this isn't to fix the size to an exact number, but to ensure we can fit 11 objects in a single page. (and 11 * 372 == 4092, so that's the absolute maximum size to fix 11 objects in 4096 bytes).

I don't have very strong views here though.

This revision is now accepted and ready to land.Fri, Nov 29, 12:38 PM
sys/net/pfvar.h
1115

I think I've put here the value read from pahole by mistake. Let me restore the original line.

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

This revision now requires review to proceed.Fri, Nov 29, 1:33 PM
This revision is now accepted and ready to land.Fri, Nov 29, 1:39 PM
This revision was automatically updated to reflect the committed changes.
vegeta_tuxpowered.net marked an inline comment as done.