From: Alexander Bluhm Subject: Re: pf af-to breaks traceroute To: Alexandr Nedvedicky Cc: Kristof Provost , tech@openbsd.org Date: Fri, 28 Feb 2025 16:15:28 +0100 This diff break 2 test cases in regress/sys/net/pf_forward *** Error 1 in . (Makefile:269 'run-traceroute-icmp-inet-AF_IN') FAILED -- *** Error 1 in . (Makefile:178 'run-ping-inet6-AF_IN') FAILED -- *** Error 1 in . (Makefile:269 'run-traceroute-icmp-inet6-AF_IN') An obvious problem is that it changes the source addres of the echo reply packet. root@ot1:.../pf_forward# make run-ping-inet6-AF_IN Check ping AF_IN6: ping6 -n -c 1 fdd7:e83e:66bc:87::18 PING fdd7:e83e:66bc:87::18 (fdd7:e83e:66bc:87::18): 56 data bytes --- fdd7:e83e:66bc:87::18 ping statistics --- 1 packets transmitted, 0 packets received, 100.0% packet loss *** Error 1 in /usr/src/regress/sys/net/pf_forward (Makefile:178 'run-ping-inet6-AF_IN') tcpdump on PF host: 16:08:43.074888 fdd7:e83e:66bc:81::21 > fdd7:e83e:66bc:87::18: icmp6: echo request 16:08:43.074986 10.188.82.22 > 10.188.83.24: icmp: echo request (DF) 16:08:43.075318 10.188.83.24 > 10.188.82.22: icmp: echo reply (DF) 16:08:43.075383 fdd7:e83e:66bc:87::abc:5318 > fdd7:e83e:66bc:81::21: icmp6: echo reply 16:08:43.075534 fdd7:e83e:66bc:81::21 > fdd7:e83e:66bc:87::abc:5318: icmp6: fdd7:e83e:66bc:81::21 protocol 58 port 16085 unreachable The address fdd7:e83e:66bc:87::abc:5318 is not recocnized by the pinging host. While translated source address is good for traceroute, ping does not work that way. Also in the final chunk if (nk->af == AF_INET) { pd->proto = IPPROTO_ICMP; is not necessary. We are in case case IPPROTO_ICMP, note that there is an unconditinal pd->proto = IPPROTO_ICMPV6 a few lines above. Maybe you should net reorder the code to keep it consistent with case IPPROTO_ICMPV6. In general I like your idea. bluhm On Thu, Feb 27, 2025 at 02:22:16AM +0100, Alexandr Nedvedicky wrote: > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/pf.c b/sys/net/pf.c > index 6d6dbfd3c14..ce2cb7c2a9c 100644 > --- a/sys/net/pf.c > +++ b/sys/net/pf.c > @@ -5650,6 +5650,16 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, > if (afto) { > pf_addrcpy(&pd->nsaddr, &nk->addr[sidx], > nk->af); > + if (nk->af == AF_INET6) { > + /* > + * IPv4 becomes IPv6 so we must put > + * IPv4 src addr to least 32bits in > + * IPv6 address to keep traceroute/icmp > + * working. > + */ > + pd->nsaddr.addr32[3] = > + pd->src->addr32[0]; > + } > pf_addrcpy(&pd->ndaddr, &nk->addr[didx], > nk->af); > pd->naf = nk->af; > @@ -5925,17 +5935,27 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, > pd, &pd2, &nk->addr[sidx], > &nk->addr[didx], pd->af, nk->af)) > return (PF_DROP); > - if (nk->af == AF_INET) > - pd->proto = IPPROTO_ICMP; > - else > - pd->proto = IPPROTO_ICMPV6; > pd->m->m_pkthdr.ph_rtableid = > nk->rdomain; > pd->destchg = 1; > - pf_addrcpy(&pd->nsaddr, > - &nk->addr[pd2.sidx], nk->af); > pf_addrcpy(&pd->ndaddr, > &nk->addr[pd2.didx], nk->af); > + pf_addrcpy(&pd->nsaddr, > + &nk->addr[pd2.sidx], nk->af); > + if (nk->af == AF_INET) { > + pd->proto = IPPROTO_ICMP; > + } else { > + pd->proto = IPPROTO_ICMPV6; > + /* > + * IPv4 becomes IPv6 so we must > + * put IPv4 src addr to least > + * 32bits in IPv6 address to > + * keep traceroute/icmp > + * working. > + */ > + pd->nsaddr.addr32[3] = > + pd->src->addr32[0]; > + } > pd->naf = nk->af; > > pf_patch_16(pd, > @@ -6045,17 +6065,27 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, > pd, &pd2, &nk->addr[sidx], > &nk->addr[didx], pd->af, nk->af)) > return (PF_DROP); > - if (nk->af == AF_INET) > - pd->proto = IPPROTO_ICMP; > - else > - pd->proto = IPPROTO_ICMPV6; > pd->m->m_pkthdr.ph_rtableid = > nk->rdomain; > pd->destchg = 1; > - pf_addrcpy(&pd->nsaddr, > - &nk->addr[pd2.sidx], nk->af); > pf_addrcpy(&pd->ndaddr, > &nk->addr[pd2.didx], nk->af); > + pf_addrcpy(&pd->nsaddr, > + &nk->addr[pd2.sidx], nk->af); > + if (nk->af == AF_INET) { > + pd->proto = IPPROTO_ICMP; > + } else { > + pd->proto = IPPROTO_ICMPV6; > + /* > + * IPv4 becomes IPv6 so we must > + * put IPv4 src addr to least > + * 32bits in IPv6 address to > + * keep traceroute/icmp > + * working. > + */ > + pd->nsaddr.addr32[3] = > + pd->src->addr32[0]; > + } > pd->naf = nk->af; > > pf_patch_16(pd, > @@ -6182,10 +6212,24 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, > pd->m->m_pkthdr.ph_rtableid = > nk->rdomain; > pd->destchg = 1; > - pf_addrcpy(&pd->nsaddr, > - &nk->addr[pd2.sidx], nk->af); > pf_addrcpy(&pd->ndaddr, > &nk->addr[pd2.didx], nk->af); > + pf_addrcpy(&pd->nsaddr, > + &nk->addr[pd2.sidx], nk->af); > + if (nk->af == AF_INET) { > + pd->proto = IPPROTO_ICMP; > + } else { > + pd->proto = IPPROTO_ICMPV6; > + /* > + * IPv4 becomes IPv6 so we must > + * put IPv4 src addr to least > + * 32bits in IPv6 address to > + * keep traceroute/icmp > + * working. > + */ > + pd->nsaddr.addr32[3] = > + pd->src->addr32[0]; > + } > pd->naf = nk->af; > return (PF_AFRT); > }