Hi,
collapse_redirspec is one of my pet peeve since the af-to support.
Unfortunately we didn't put much effort in making it work well back
then, but now it is time for a change! Improving upon the last
diff here's a collapsed version of the collapse_redirspec, so to
speak. Instead of having two loops: first counts addresses, the
second one performs an action, here's a merged version that has all
the checks done once and in the right order.
No changes in the regress test output (all tests pass) and the
gain is that we no longer need to explicitely set the address
family for af-to rules, it will be deduced in most of the cases.
For example, previously you'd have to do:
pass in inet af-to inet6 from ::1
While it's clear that you're translating IPv4 to the IPv6. Now
it's possible to omit the 'inet' part:
pass in af-to inet6 from ::1
Which is in line with how nat-to and rdr-to target addresses
affect selection of the address family for the whole rule, i.e.:
% echo 'pass in rdr-to ::1' | pfctl -vnf -
pass in inet6 all flags S/SA rdr-to ::1
(Of course af-to rule still needs the 'inet6' part, but that
will require some other changes as well...)
OK?
P.S. This diff moves chunks around quite a bit and might require
applying to tell the difference.
diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index ce80f8e..fabe210 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -2025,26 +2025,21 @@ filter_opt : USER uids {
filter_opts.nat.rdr = $2;
memcpy(&filter_opts.nat.pool_opts, &$3,
sizeof(filter_opts.nat.pool_opts));
}
| AFTO af FROM redirpool pool_opts {
if (filter_opts.nat.rdr) {
yyerror("cannot respecify af-to");
YYERROR;
}
if ($2 == 0) {
- yyerror("no address family specified");
- YYERROR;
- }
- if ($4->host->af && $4->host->af != $2) {
- yyerror("af-to addresses must be in the "
- "target address family");
+ yyerror("no target address family specified");
YYERROR;
}
filter_opts.nat.af = $2;
filter_opts.nat.rdr = $4;
memcpy(&filter_opts.nat.pool_opts, &$5,
sizeof(filter_opts.nat.pool_opts));
filter_opts.rdr.rdr =
calloc(1, sizeof(struct redirection));
bzero(&filter_opts.rdr.pool_opts,
sizeof(filter_opts.rdr.pool_opts));
@@ -3482,21 +3477,21 @@ redirspec : host optweight {
n->weight = $2;
}
$$ = $1;
}
| '{' optnl redir_host_list '}' { $$ = $3; }
;
redir_host_list : host optweight optnl {
if ($1->addr.type != PF_ADDR_ADDRMASK) {
free($1);
- yyerror("only addresses can be listed for"
+ yyerror("only addresses can be listed for "
"redirection pools ");
YYERROR;
}
if ($2 > 0) {
struct node_host *n;
for (n = $1; n != NULL; n = n->next)
n->weight = $2;
}
$$ = $1;
}
@@ -4288,118 +4283,139 @@ expand_queue(char *qname, struct node_if *interfaces,
struct queue_opts *opts)
FREE_LIST(struct node_if, interfaces);
return (0);
}
int
collapse_redirspec(struct pf_pool *rpool, struct pf_rule *r,
struct redirspec *rs, u_int8_t allow_if)
{
struct pf_opt_tbl *tbl = NULL;
- struct node_host *h;
+ struct node_host *h, *hprev = NULL;
struct pf_rule_addr ra;
- int af = 0, i = 0;
+ int af = 0, naddr = 0;
if (!rs || !rs->rdr || rs->rdr->host == NULL) {
rpool->addr.type = PF_ADDR_NONE;
return (0);
}
if (r->rule_flag & PFRULE_AFTO)
r->naf = rs->af;
- /* count matching addresses */
for (h = rs->rdr->host; h != NULL; h = h->next) {
- if (h->af) {
- /* check that af-to target address has correct af */
- if (rs->af && rs->af != h->af) {
- yyerror("af mismatch in %s spec",
- allow_if ? "routing" : "translation");
- return (1);
- }
+ /* set rule address family if redirect spec has one */
+ if (rs->af && !r->af && !af) {
+ /* swap address families for af-to */
+ if (r->naf == AF_INET6 && rs->af == AF_INET6)
+ af = AF_INET;
+ else if (r->naf == AF_INET && rs->af == AF_INET)
+ af = AF_INET6;
+ else
+ af = rs->af;
+ }
+ if (h->af && !r->naf) { /* nat-to/rdr-to case */
/* skip if the rule af doesn't match redirect af */
- if ((r->af && r->af != h->af) && /* exclude af-to */
- !(r->naf && h->af == r->naf))
+ if (r->af && r->af != h->af)
continue;
/*
* fail if the chosen af is not universal for the
- * whole supplied redirect address pool
+ * all addresses in the redirect address pool
*/
if (!r->af && af && af != h->af) {
yyerror("%s spec contains addresses with "
"different address families",
allow_if ? "routing" : "translation");
return (1);
}
- if (!r->af)
- af = h->af;
- i++;
- } else {
+ } else if (h->af) { /* af-to case */
/*
- * we silently allow any not af-specific host specs,
- * e.g. (em0) and let the kernel deal with them
+ * fail if the redirect spec af is not universal
+ * for all addresses in the redirect address pool
*/
- i++;
+ if (rs->af && rs->af != h->af) {
+ yyerror("%s spec contains addresses that "
+ "don't match target address family",
+ allow_if ? "routing" : "translation");
+ return (1);
+ }
}
- }
- /* set rule af to one chosen above */
- if (!r->af && af)
- r->af = af;
+ /* else if (!h->af):
+ * we silently allow any not af-specific host specs,
+ * e.g. (em0) and let the kernel deal with them
+ */
- if (i == 0) { /* no pool address */
- yyerror("af mismatch in %s spec",
- allow_if ? "routing" : "translation");
- return (1);
- } else if (i == 1) { /* only one address */
- for (h = rs->rdr->host; h != NULL; h = h->next)
- if (!h->af || !r->af || rs->af || r->af == h->af ||
- (r->naf && r->naf == h->af))
- break;
- rpool->addr = h->addr;
- if (!allow_if && h->ifname) {
- yyerror("@if not permitted for translation");
- return (1);
- }
- if (h->ifname && strlcpy(rpool->ifname, h->ifname,
- sizeof(rpool->ifname)) >= sizeof(rpool->ifname))
- errx(1, "collapse_redirspec: strlcpy");
+ /* if we haven't selected the rule af yet, now it's time */
+ if (!r->af && !af)
+ af = h->af;
- return (0);
- } else { /* more than one address */
- if (rs->pool_opts.type &&
- (rs->pool_opts.type != PF_POOL_ROUNDROBIN) &&
- (rs->pool_opts.type != PF_POOL_LEASTSTATES)) {
- yyerror("only round-robin or "
- "least-states valid for multiple "
- "translation or routing addresses");
- return (1);
- }
- for (h = rs->rdr->host; h != NULL; h = h->next) {
- if (!rs->af && r->af != h->af)
- continue;
- if (h->addr.type != PF_ADDR_ADDRMASK &&
+ if (naddr == 0) { /* the first host */
+ rpool->addr = h->addr;
+ if (!allow_if && h->ifname) {
+ yyerror("@if not permitted for translation");
+ return (1);
+ }
+ if (h->ifname && strlcpy(rpool->ifname, h->ifname,
+ sizeof(rpool->ifname)) >= sizeof(rpool->ifname))
+ errx(1, "collapse_redirspec: strlcpy");
+ hprev = h; /* in case we need to conver to a table */
+ } else { /* multiple hosts */
+ if (rs->pool_opts.type &&
+ (rs->pool_opts.type != PF_POOL_ROUNDROBIN) &&
+ (rs->pool_opts.type != PF_POOL_LEASTSTATES)) {
+ yyerror("only round-robin or "
+ "least-states valid for multiple "
+ "translation or routing addresses");
+ return (1);
+ }
+ if ((hprev && hprev->addr.type != PF_ADDR_ADDRMASK) &&
+ (hprev && hprev->addr.type != PF_ADDR_NONE) &&
+ h->addr.type != PF_ADDR_ADDRMASK &&
h->addr.type != PF_ADDR_NONE) {
yyerror("multiple tables or dynamic interfaces "
"not supported for translation or routing");
return (1);
}
if (!allow_if && h->ifname) {
yyerror("@if not permitted for translation");
return (1);
}
+ if (hprev) {
+ /*
+ * undo some damage and convert the single
+ * host pool to the table
+ */
+ memset(&ra, 0, sizeof(ra));
+ memset(rpool->ifname, 0, sizeof(rpool->ifname));
+ ra.addr = hprev->addr;
+ ra.weight = hprev->weight;
+ if (add_opt_table(pf, &tbl,
+ hprev->af, &ra, hprev->ifname))
+ return (1);
+ hprev = NULL;
+ }
memset(&ra, 0, sizeof(ra));
ra.addr = h->addr;
ra.weight = h->weight;
if (add_opt_table(pf, &tbl,
h->af, &ra, h->ifname))
return (1);
}
+ naddr++;
+ }
+ /* set rule af to the one chosen above */
+ if (!r->af && af)
+ r->af = af;
+ if (!naddr) {
+ yyerror("af mismatch in %s spec",
+ allow_if ? "routing" : "translation");
+ return (1);
}
if (tbl) {
if ((pf->opts & PF_OPT_NOACTION) == 0 &&
pf_opt_create_table(pf, tbl))
return (1);
pf->tdirty = 1;
if (pf->opts & PF_OPT_VERBOSE)
print_tabledef(tbl->pt_name,
