From: Claudio Jeker Subject: Re: route cache mpath To: Alexander Bluhm Cc: tech@openbsd.org Date: Wed, 28 Feb 2024 09:07:48 +0100 On Tue, Feb 27, 2024 at 06:27:53PM +0100, Alexander Bluhm wrote: > On Tue, Feb 27, 2024 at 02:19:29PM +0100, Claudio Jeker wrote: > > On Tue, Feb 27, 2024 at 01:52:51PM +0100, Alexander Bluhm wrote: > > > On Mon, Feb 26, 2024 at 11:44:57AM +0100, Alexander Bluhm wrote: > > > > Also IP input should pass a struct route to IP forward. This is > > > > the same logic that is done when passing a route from IP forward > > > > to IP output. As a result the numbers of route cache lookups in > > > > netstat -s should be correct now. > > > > > > > > Finally I removed some inconsistencies between IPv4 and IPv4 and > > > > IP forward and IP output. > > > > > > > Or should I split the diff in smaller pieces? > > > > > > This is the other half of the diff. > > > > > > ok? > > > > Some bikesheds inline. > > In general the diff should go, OK claudio@ > > > > -- > > :wq Claudio > > > > > @@ -1482,26 +1480,23 @@ ip_forward(struct mbuf *m, struct ifnet > > > if (m->m_flags & (M_BCAST|M_MCAST) || in_canforward(ip->ip_dst) == 0) { > > > ipstat_inc(ips_cantforward); > > > m_freem(m); > > > - goto freecopy; > > > + goto done; > > > > Would have been good to do that as a seperate step since it adds a lot of > > noise to the diff. > > Sure, here is the cleanup part. > > > > + if (rt == NULL) { > > > + ip6stat_inc(ip6s_noroute); > > > + if (mcopy) { > > > > I would prefer to use if (mcopy != NULL) here. See more below. > > Yes, let's make it consistent the other way around. > > > Same here. I prefer if (mcopy != NULL) over if (mcopy) for pointers. > > Especially since a few lines below we have: if (mcopy == NULL) > > Now I have changed all of them. > > I have put a space in front of the goto labels. Then diff -p shows > the function name and not the label. > > ok? OK claudio@ > bluhm > > Index: netinet/ip_input.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v > diff -u -p -r1.390 ip_input.c > --- netinet/ip_input.c 22 Feb 2024 14:25:58 -0000 1.390 > +++ netinet/ip_input.c 27 Feb 2024 15:55:47 -0000 > @@ -391,7 +391,10 @@ ip_input_if(struct mbuf **mp, int *offp, > struct rtentry *rt = NULL; > struct ip *ip; > int hlen; > - in_addr_t pfrdr = 0; > +#if NPF > 0 > + struct in_addr odst; > +#endif > + int pfrdr = 0; > > KASSERT(*offp == 0); > > @@ -412,7 +415,7 @@ ip_input_if(struct mbuf **mp, int *offp, > /* > * Packet filter > */ > - pfrdr = ip->ip_dst.s_addr; > + odst = ip->ip_dst; > if (pf_test(AF_INET, PF_IN, ifp, mp) != PF_PASS) > goto bad; > m = *mp; > @@ -420,7 +423,7 @@ ip_input_if(struct mbuf **mp, int *offp, > goto bad; > > ip = mtod(m, struct ip *); > - pfrdr = (pfrdr != ip->ip_dst.s_addr); > + pfrdr = odst.s_addr != ip->ip_dst.s_addr; > #endif > > hlen = ip->ip_hl << 2; > @@ -1472,7 +1475,7 @@ const u_char inetctlerrmap[PRC_NCMDS] = > void > ip_forward(struct mbuf *m, struct ifnet *ifp, struct rtentry *rt, int srcrt) > { > - struct mbuf mfake, *mcopy = NULL; > + struct mbuf mfake, *mcopy; > struct ip *ip = mtod(m, struct ip *); > struct route ro; > int error = 0, type = 0, code = 0, destmtu = 0, fake = 0, len; > @@ -1482,11 +1485,11 @@ ip_forward(struct mbuf *m, struct ifnet > if (m->m_flags & (M_BCAST|M_MCAST) || in_canforward(ip->ip_dst) == 0) { > ipstat_inc(ips_cantforward); > m_freem(m); > - goto freecopy; > + goto done; > } > if (ip->ip_ttl <= IPTTLDEC) { > icmp_error(m, ICMP_TIMXCEED, ICMP_TIMXCEED_INTRANS, dest, 0); > - goto freecopy; > + goto done; > } > > ro.ro_rt = NULL; > @@ -1563,10 +1566,10 @@ ip_forward(struct mbuf *m, struct ifnet > if (type) > ipstat_inc(ips_redirectsent); > else > - goto freecopy; > + goto done; > } > if (!fake) > - goto freecopy; > + goto done; > > switch (error) { > case 0: /* forwarded, but need redirect */ > @@ -1590,7 +1593,7 @@ ip_forward(struct mbuf *m, struct ifnet > } > ipstat_inc(ips_cantfrag); > if (destmtu == 0) > - goto freecopy; > + goto done; > break; > > case EACCES: > @@ -1598,7 +1601,7 @@ ip_forward(struct mbuf *m, struct ifnet > * pf(4) blocked the packet. There is no need to send an ICMP > * packet back since pf(4) takes care of it. > */ > - goto freecopy; > + goto done; > > case ENOBUFS: > /* > @@ -1607,7 +1610,7 @@ ip_forward(struct mbuf *m, struct ifnet > * source quench could be a big problem under DoS attacks, > * or the underlying interface is rate-limited. > */ > - goto freecopy; > + goto done; > > case ENETUNREACH: /* shouldn't happen, checked above */ > case EHOSTUNREACH: > @@ -1619,10 +1622,10 @@ ip_forward(struct mbuf *m, struct ifnet > break; > } > mcopy = m_copym(&mfake, 0, len, M_DONTWAIT); > - if (mcopy) > + if (mcopy != NULL) > icmp_error(mcopy, type, code, dest, destmtu); > > -freecopy: > + done: > if (fake) > m_tag_delete_chain(&mfake); > rtfree(rt); > Index: netinet6/ip6_forward.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_forward.c,v > diff -u -p -r1.115 ip6_forward.c > --- netinet6/ip6_forward.c 22 Feb 2024 14:25:58 -0000 1.115 > +++ netinet6/ip6_forward.c 27 Feb 2024 15:43:48 -0000 > @@ -89,7 +89,7 @@ ip6_forward(struct mbuf *m, struct rtent > struct route ro; > struct ifnet *ifp = NULL; > int error = 0, type = 0, code = 0, destmtu = 0; > - struct mbuf *mcopy = NULL; > + struct mbuf *mcopy; > #ifdef IPSEC > struct tdb *tdb = NULL; > #endif /* IPSEC */ > @@ -121,13 +121,13 @@ ip6_forward(struct mbuf *m, struct rtent > m->m_pkthdr.ph_ifidx); > } > m_freem(m); > - goto out; > + goto done; > } > > if (ip6->ip6_hlim <= IPV6_HLIMDEC) { > icmp6_error(m, ICMP6_TIME_EXCEEDED, > ICMP6_TIME_EXCEED_TRANSIT, 0); > - goto out; > + goto done; > } > ip6->ip6_hlim -= IPV6_HLIMDEC; > > @@ -175,12 +175,12 @@ reroute: > m->m_pkthdr.ph_rtableid); > if (rt == NULL) { > ip6stat_inc(ip6s_noroute); > - if (mcopy) { > + if (mcopy != NULL) { > icmp6_error(mcopy, ICMP6_DST_UNREACH, > ICMP6_DST_UNREACH_NOROUTE, 0); > } > m_freem(m); > - goto out; > + goto done; > } > } > ro.ro_rt = rt; > @@ -211,11 +211,11 @@ reroute: > ip6->ip6_nxt, > m->m_pkthdr.ph_ifidx, rt->rt_ifidx); > } > - if (mcopy) > + if (mcopy != NULL) > icmp6_error(mcopy, ICMP6_DST_UNREACH, > ICMP6_DST_UNREACH_BEYONDSCOPE, 0); > m_freem(m); > - goto out; > + goto done; > } > > #ifdef IPSEC > @@ -270,11 +270,11 @@ reroute: > * type/code is based on suggestion by Rich Draves. > * not sure if it is the best pick. > */ > - if (mcopy) > + if (mcopy != NULL) > icmp6_error(mcopy, ICMP6_DST_UNREACH, > ICMP6_DST_UNREACH_ADDR, 0); > m_freem(m); > - goto out; > + goto done; > } > type = ND_REDIRECT; > } > @@ -327,18 +327,18 @@ reroute: > if (mcopy != NULL) > icmp6_error(mcopy, ICMP6_PACKET_TOO_BIG, 0, ifp->if_mtu); > m_freem(m); > - goto out; > + goto done; > > senderr: > if (mcopy == NULL) > - goto out; > + goto done; > > switch (error) { > case 0: > if (type == ND_REDIRECT) { > icmp6_redirect_output(mcopy, rt); > ip6stat_inc(ip6s_redirectsent); > - goto out; > + goto done; > } > goto freecopy; > > @@ -383,11 +383,11 @@ senderr: > break; > } > icmp6_error(mcopy, type, code, destmtu); > - goto out; > + goto done; > > -freecopy: > + freecopy: > m_freem(mcopy); > -out: > + done: > rtfree(rt); > if_put(ifp); > #ifdef IPSEC > Index: netinet6/ip6_input.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v > diff -u -p -r1.258 ip6_input.c > --- netinet6/ip6_input.c 22 Feb 2024 14:25:58 -0000 1.258 > +++ netinet6/ip6_input.c 27 Feb 2024 15:52:54 -0000 > @@ -366,7 +366,7 @@ ip6_input_if(struct mbuf **mp, int *offp > #if NPF > 0 > struct in6_addr odst; > #endif > - int srcrt = 0; > + int pfrdr = 0; > > KASSERT(*offp == 0); > > @@ -413,7 +413,7 @@ ip6_input_if(struct mbuf **mp, int *offp > goto bad; > > ip6 = mtod(m, struct ip6_hdr *); > - srcrt = !IN6_ARE_ADDR_EQUAL(&odst, &ip6->ip6_dst); > + pfrdr = !IN6_ARE_ADDR_EQUAL(&odst, &ip6->ip6_dst); > #endif > > /* > @@ -618,7 +618,7 @@ ip6_input_if(struct mbuf **mp, int *offp > } > #endif /* IPSEC */ > > - ip6_forward(m, rt, srcrt); > + ip6_forward(m, rt, pfrdr); > *mp = NULL; > return IPPROTO_DONE; > bad: > Index: netinet6/ip6_output.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_output.c,v > diff -u -p -r1.287 ip6_output.c > --- netinet6/ip6_output.c 22 Feb 2024 14:25:58 -0000 1.287 > +++ netinet6/ip6_output.c 27 Feb 2024 15:50:45 -0000 > @@ -748,8 +748,16 @@ reroute: > (error = if_output_ml(ifp, &ml, sin6tosa(dst), ro->ro_rt))) > goto done; > ip6stat_inc(ip6s_fragmented); > + goto done; > > -done: > + freehdrs: > + m_freem(exthdrs.ip6e_hbh); /* m_freem will check if mbuf is 0 */ > + m_freem(exthdrs.ip6e_dest1); > + m_freem(exthdrs.ip6e_rthdr); > + m_freem(exthdrs.ip6e_dest2); > + bad: > + m_freem(m); > + done: > if (ro == &iproute && ro->ro_rt) { > rtfree(ro->ro_rt); > } else if (ro_pmtu == &iproute && ro_pmtu->ro_rt) { > @@ -760,16 +768,6 @@ done: > tdb_unref(tdb); > #endif /* IPSEC */ > return (error); > - > -freehdrs: > - m_freem(exthdrs.ip6e_hbh); /* m_freem will check if mbuf is 0 */ > - m_freem(exthdrs.ip6e_dest1); > - m_freem(exthdrs.ip6e_rthdr); > - m_freem(exthdrs.ip6e_dest2); > - /* FALLTHROUGH */ > -bad: > - m_freem(m); > - goto done; > } > > int > -- :wq Claudio