Page MenuHomeFreeBSD

pf: partially import OpenBSD's NAT rewrite
Needs ReviewPublic

Authored by kp on Wed, Nov 27, 4:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 12:43 AM
Unknown Object (File)
Thu, Dec 5, 8:55 PM
Unknown Object (File)
Sat, Nov 30, 11:06 PM

Details

Reviewers
None
Group Reviewers
pfsense
network
Summary

We won't follow this fully, because it involves breaking syntax changes
(removing nat/rdr rules and moving this functionality into regular rules) as
well as behaviour changes because NAT is now done after the rules evaluation,
rather than before it.

We import some related changes anyway, because it paves the way for nat64
support.
This change introduces a new pf_kpool in struct pf_krule, for nat. It is not yet
used (but will be for nat64) and renames the existing 'rpool' to 'rdr'.

Obtained from: OpenBSD, henning <henning@openbsd.org>, 0ef3d4febe
Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

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

Event Timeline

kp requested review of this revision.Wed, Nov 27, 4:41 PM
lib/libpfctl/libpfctl.c
1230

Let's use less cryptic names. Both are redirection pools, one used for NAT the other for route targets. Why not rpool_nat and rpool_route?

sys/net/pfvar.h
786

The same question as for netlink: how about some better names like rpool_nat and rpool_route? Or even better, since there is a new which, maybe we could have a new enum (I see that currently which abuses the enum for rule type) for rpool type and have here an array?

lib/libpfctl/libpfctl.c
1230

Do you mean for the netlink name (so say PF_RT_RPOOL_NAT / PF_RT_RPOOL_ROUTE) or for the structure pf_krule fields?

The former I'm happy to change (well, the PF_RT_NAT one anyway, PF_RT_RPOOL is already in live code. Admittedly only on the main branch, so we could still get away with changing it, but still), but the latter is what OpenBSD uses and I'd prefer to not create conflicts unnecessarily.

sys/net/pfvar.h
786

As above, I'd prefer to keep this close to the OpenBSD code if possible.
I also don't think we need an array we're extremely unlikely to ever extend.

lib/libpfctl/libpfctl.c
1230

PF_RT_RPOOL_NAT and PF_RT_RPOOL_ROUTE sound good to me. The _RPOOL part nicely groups them together and shows what the underlying struct is about, and _ROUTE and `_NAT clearly show what each pool is used for.

Admittedly only on the main branch, so we could still get away with changing it, but still.

So there was no release yet. If you have any testing systems which must undergo upgrades you could add the new netlink variable, read both, keep it for a few weeks, delete it later.

but the latter is what OpenBSD uses and I'd prefer to not create conflicts unnecessarily.

OpenBSD does not use netlink, what do you mean? There are

struct pf_rule {
…
	struct pf_pool		 nat;
	struct pf_pool		 rdr;
	struct pf_pool		 route;
…
}

in OpenBSD, so we could easily do:
PF_RT_RPOOL_NAT -> struct pf_pool nat
PF_RT_RPOOL_ROUTE -> struct pf_pool route

And that leaves me free to pursue
PF_RT_RPOOL_RDR -> struct pf_pool rdr
one day.

sys/net/pfvar.h
786

The OpenBSD argument is good. Let's keep their names for struct pf_krule members.

lib/libpfctl/libpfctl.c
1230

OpenBSD does not use netlink, what do you mean? There are

I was talking about the struct pf_krule members, not the netlink variable name here.
I think we're in agreement. I'll work on updating the netlink names.

rename PF_RT_RPOOL/PF_RT_NAT