On Thu, Jun 23, 2016 at 00:38 +0200, Alexander Bluhm wrote:
> On Wed, Jun 22, 2016 at 08:15:09PM +0200, Mike Belopuhov wrote:
> > Can you or benno test NAT64 with this change?
> > In case of weird behavior do this:
> >
> > int sidx = pd->af == pd->naf ? pd->sidx : pd->didx;
> > int didx = pd->af == pd->naf ? pd->didx : pd->sidx;
> >
> > And use sidx/didx throughout instead of pd->sidx and pd->didx.
> >
> > I'm pretty sure you need to do this trick, but I'm not 100%
> > certain.
>
> af-to state lookup in pf_get_sport() is quite broken.
>
> Jun 23 00:25:26 q70 /bsd: pf: af-to inet6 rdr, 10.188.70.17:3003 ->
> 10.188.216.114:7
> Jun 23 00:25:26 q70 /bsd: pf: find state all dir=out, af=24, key0:
> fdd7:e83e:66bc:211:725f:caff:fe21:8d70[10001], key1: abc:d872::[7], proto=17
> Jun 23 00:25:26 q70 /bsd: pf: af-to inet6 rdr done, prefixlen 120,
> fdd7:e83e:66bc:211:725f:caff:fe21:8d70[10001] ->
> fdd7:e83e:66bc:212:725f:caff:fe21:8d72[7]
>
> Look at the key1: abc:d872::[7], that is the IPv4 address used as IPv6.
> pf_get_transaddr_af() will fix the prefix later.
>
Looks like the pd->ndaddr/nsaddr patching should happen before
calling pf_get_sport.
> As there is more work to be done for af-to, I propose this version
> of the nat-to fix. With the explicit variables sidx and didx we
> can swap it easily if we will need it.
>
Sure, OK mikeb.
> bluhm
>
> Index: net/pf_lb.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_lb.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 pf_lb.c
> --- net/pf_lb.c 15 Jun 2016 11:36:06 -0000 1.53
> +++ net/pf_lb.c 22 Jun 2016 22:18:30 -0000
> @@ -155,6 +155,9 @@ pf_get_sport(struct pf_pdesc *pd, struct
> struct pf_state_key_cmp key;
> struct pf_addr init_addr;
> u_int16_t cut;
> + int dir = (pd->dir == PF_IN) ? PF_OUT : PF_IN;
> + int sidx = pd->sidx;
> + int didx = pd->didx;
>
> bzero(&init_addr, sizeof(init_addr));
> if (pf_map_addr(pd->naf, r, &pd->nsaddr, naddr, &init_addr, sn, &r->nat,
> @@ -182,9 +185,9 @@ pf_get_sport(struct pf_pdesc *pd, struct
> key.af = pd->naf;
> key.proto = pd->proto;
> key.rdomain = pd->rdomain;
> - PF_ACPY(&key.addr[0], &pd->ndaddr, key.af);
> - PF_ACPY(&key.addr[1], naddr, key.af);
> - key.port[0] = pd->ndport;
> + PF_ACPY(&key.addr[didx], &pd->ndaddr, key.af);
> + PF_ACPY(&key.addr[sidx], naddr, key.af);
> + key.port[didx] = pd->ndport;
>
> /*
> * port search; start random, step;
> @@ -194,20 +197,20 @@ pf_get_sport(struct pf_pdesc *pd, struct
> pd->proto == IPPROTO_ICMP || pd->proto == IPPROTO_ICMPV6)) {
> /* XXX bug: icmp states dont use the id on both
> * XXX sides (traceroute -I through nat) */
> - key.port[1] = pd->nsport;
> - if (pf_find_state_all(&key, PF_IN, NULL) == NULL) {
> + key.port[sidx] = pd->nsport;
> + if (pf_find_state_all(&key, dir, NULL) == NULL) {
> *nport = pd->nsport;
> return (0);
> }
> } else if (low == 0 && high == 0) {
> - key.port[1] = pd->nsport;
> - if (pf_find_state_all(&key, PF_IN, NULL) == NULL) {
> + key.port[sidx] = pd->nsport;
> + if (pf_find_state_all(&key, dir, NULL) == NULL) {
> *nport = pd->nsport;
> return (0);
> }
> } else if (low == high) {
> - key.port[1] = htons(low);
> - if (pf_find_state_all(&key, PF_IN, NULL) == NULL) {
> + key.port[sidx] = htons(low);
> + if (pf_find_state_all(&key, dir, NULL) == NULL) {
> *nport = htons(low);
> return (0);
> }
> @@ -223,16 +226,16 @@ pf_get_sport(struct pf_pdesc *pd, struct
> cut = arc4random_uniform(1 + high - low) + low;
> /* low <= cut <= high */
> for (tmp = cut; tmp <= high; ++(tmp)) {
> - key.port[1] = htons(tmp);
> - if (pf_find_state_all(&key, PF_IN, NULL) ==
> + key.port[sidx] = htons(tmp);
> + if (pf_find_state_all(&key, dir, NULL) ==
> NULL && !in_baddynamic(tmp, pd->proto)) {
> *nport = htons(tmp);
> return (0);
> }
> }
> for (tmp = cut - 1; tmp >= low; --(tmp)) {
> - key.port[1] = htons(tmp);
> - if (pf_find_state_all(&key, PF_IN, NULL) ==
> + key.port[sidx] = htons(tmp);
> + if (pf_find_state_all(&key, dir, NULL) ==
> NULL && !in_baddynamic(tmp, pd->proto)) {
> *nport = htons(tmp);
> return (0);
