Hello,
patch which fixes the problem is below. The issue has been sort of introduced
by my earlier commit [1] to fix another problem reported by giovanni@.
it has actually turned out my earlier change was not complete and actually
made a problem reported worse.
the original problem reported by giovanni@ shows symptoms as follows:
the nat rule:
pass out from 192.168.1.0/24 to any nat-to { 49.0.0.1 } round-robin
translates matching packets to:
0.0.1.243, 0.0.1.244, ...
which is completely wrong. My commit [2] fixes that so we get expected
result: all matching packets get translated to 49.0.0.1
unfortunately earlier change [2] makes things worse for rule reported
by Hrvoje:
pass out from 192.168.1.0/24 to any nat-to { 49/27 } round-robin
to understand how things end up such badly we have to take a look
at how 'nat-to' argument is handled in pf_lb.c. There are three possible
ways how nat-to argument is interpreted:
a) radix table
if 'nat-to' refers to interface: 'nat-to em0'
b) dynamic table
if 'nat-to' refers to interface like that: 'nat-to (em0)'
c) single IP address
if 'nat-to' is written as follows: 'nat-to { 1.2.3.4 }'
d) table of IP addresses:
'nat-to {1.2.3.4, 10.20.30.40}'
e) single IP address which represents network:
'nat-to {49/27}'
all forms above are implemented as 'struct pf_pool' in pf(4).
variants 'nat-to {1.2.3.4}' and 'nat-to 1.2.3.4' are equal the
resulting pf_pool instance is same. Variants a, b, d are
not interesting for us, because bug occurs in variants c and e.
to keep c and e working those three actors:
pf_pool.addr.v.a.addr
pf_pool.addr.v.a.mask
pf_pool.counter
must play well when 'round-robin' option is specified. here
the pf_pool.counter remembers the last address 'retrieved' from pool
so pf(4) can select the new address for next packet.
So in case 49/27 we are supposed to be selecting addresses:
49.0.0.1, 49.0.0.2, ..., 49.0.0.30, 49.0.0.1
we need to make sure selection mechanism skips network
address (49.0.0.0) and network broadcast address (49.0.0.31).
With my earlier commit [2] in tree this fails to happen.
As soon as we load 'nat-to {49/27}' rule to kernel the
pf_pool.counter value is 0 this triggers the condition
I've introduced [2]:
488 case PF_POOL_ROUNDROBIN:
...
501 } else if (PF_AZERO(&rpool->counter, af)) {
502 /*
503 * fall back to POOL_NONE if there are no addresses
in
504 * pool
505 */
506 pf_addrcpy(naddr, raddr, af);
507 break;
fallback action translates packet to 49.0.0.0 without updating
pf_pool.counter. The code above needs more attention:
we need to fallback if and only if pool comes with
single /32 (/128) IP address.
I still don't know why things go that wild on NET_LOCK
when pf(4) starts to translate packets to 49.0.0.0.
I have not investigated that far yet.
the glitch for {49/27} random is kind of similar. The current
code reads as follows:
408 case PF_POOL_RANDOM:
409 if (rpool->addr.type == PF_ADDR_TABLE ||
410 rpool->addr.type == PF_ADDR_DYNIFTL) {
...
427 pf_addrcpy(naddr, &rpool->counter, af);
428 } else if (init_addr != NULL && PF_AZERO(init_addr, af)) {
429 switch (af) {
430 case AF_INET:
431 rpool->counter.addr32[0] = arc4random();
432 break;
433 #ifdef INET6
434 case AF_INET6:
...
454 pf_poolmask(naddr, raddr, rmask, &rpool->counter,
af);
455 pf_addrcpy(init_addr, naddr, af);
at line 431 we use arc4random() to generate 32bit integer. at line 454 we
combine the random number with network mask (/27 in our case).
Tests were conveyed on UDP streams where 60k/sec unique UDP packets were
sent. On Hrvoje's network there were 370232 udp packets with unique source
address sent in 5secs. 11610 of them ended up to be translated to 49.0.0.0
~30% of packets we saw are translated to invalid address. Why?
because at line 431 we select random number from entire integer range,
however we must do random select on significantly smaller range:
<1, 30> in case of /27 prefix.
I've described above. It's still kind of mystery why ?net_lock? becomes
contended when we attempt to transmit 49.0.0.0
diff below addresses all issues I've described above.
OK to commit?
thanks and
regards
sashan
[1] https://marc.info/?l=openbsd-cvs&m=164500117319660&w=2
[2] https://marc.info/?l=openbsd-bugs&m=164458056429812&w=2
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf_lb.c b/sys/net/pf_lb.c
index d106073d372..5aa7f252fa3 100644
--- a/sys/net/pf_lb.c
+++ b/sys/net/pf_lb.c
@@ -344,6 +344,44 @@ pf_map_addr_sticky(sa_family_t af, struct pf_rule *r,
struct pf_addr *saddr,
return (0);
}
+uint32_t
+pf_rand_addr(uint32_t mask)
+{
+ uint32_t addr;
+
+ if (mask == 0)
+ return (arc4random());
+
+ mask = ~ntohl(mask);
+ do {
+ addr = arc4random_uniform(mask + 1);
+ } while ((addr == 0) || (addr == mask));
+
+ return (htonl(addr));
+}
+
+int
+pf_is_broadcast_addr(struct pf_addr *a, struct pf_addr *m, sa_family_t af)
+{
+ int rv;
+
+ switch (af) {
+ case AF_INET:
+ rv = (a->addr32[0] == ~m->addr32[0]);
+ break;
+ case AF_INET6:
+ rv = ((a->addr32[0] == ~m->addr32[0]) &&
+ (a->addr32[1] == ~m->addr32[1]) &&
+ (a->addr32[2] == ~m->addr32[2]) &&
+ (a->addr32[3] == ~m->addr32[3]));
+ break;
+ default:
+ unhandled_af(af);
+ }
+
+ return (rv);
+}
+
int
pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr,
struct pf_addr *naddr, struct pf_addr *init_addr, struct pf_src_node **sns,
@@ -428,24 +466,29 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
pf_addr *saddr,
} else if (init_addr != NULL && PF_AZERO(init_addr, af)) {
switch (af) {
case AF_INET:
- rpool->counter.addr32[0] = arc4random();
+ rpool->counter.addr32[0] = pf_rand_addr(
+ rmask->addr32[0]);
break;
#ifdef INET6
case AF_INET6:
if (rmask->addr32[3] != 0xffffffff)
- rpool->counter.addr32[3] = arc4random();
+ rpool->counter.addr32[3] = pf_rand_addr(
+ rmask->addr32[3]);
else
break;
if (rmask->addr32[2] != 0xffffffff)
- rpool->counter.addr32[2] = arc4random();
+ rpool->counter.addr32[2] = pf_rand_addr(
+ rmask->addr32[2]);
else
break;
if (rmask->addr32[1] != 0xffffffff)
- rpool->counter.addr32[1] = arc4random();
+ rpool->counter.addr32[1] = pf_rand_addr(
+ rmask->addr32[1]);
else
break;
if (rmask->addr32[0] != 0xffffffff)
- rpool->counter.addr32[0] = arc4random();
+ rpool->counter.addr32[0] = pf_rand_addr(
+ rmask->addr32[0]);
break;
#endif /* INET6 */
default:
@@ -500,11 +543,16 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
pf_addr *saddr,
}
} else if (PF_AZERO(&rpool->counter, af)) {
/*
- * fall back to POOL_NONE if there are no addresses in
- * pool
+ * fall back to POOL_NONE if there is a single host
+ * address in pool.
*/
- pf_addrcpy(naddr, raddr, af);
- break;
+ if ((af == AF_INET &&
+ rmask->addr32[0] == INADDR_BROADCAST) ||
+ (af == AF_INET6 &&
+ IN6_ARE_ADDR_EQUAL(&rmask->v6, &in6mask128))) {
+ pf_addrcpy(naddr, raddr, af);
+ break;
+ }
} else if (pf_match_addr(0, raddr, rmask, &rpool->counter, af))
return (1);
@@ -532,10 +580,15 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
pf_addr *saddr,
weight = rpool->weight;
}
- pf_addrcpy(naddr, &rpool->counter, af);
- if (init_addr != NULL && PF_AZERO(init_addr, af))
- pf_addrcpy(init_addr, naddr, af);
pf_addr_inc(&rpool->counter, af);
+ if (init_addr != NULL && PF_AZERO(init_addr, af))
+ pf_addrcpy(init_addr, &rpool->counter, af);
+
+ /* skip network broadcast addresses */
+ if (pf_is_broadcast_addr(&rpool->counter, rmask, af))
+ pf_addr_inc(&rpool->counter, af);
+
+ pf_poolmask(naddr, raddr, rmask, &rpool->counter, af);
break;
case PF_POOL_LEASTSTATES:
/* retrieve an address first */
