Download raw body.
route cache mpath
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
route cache mpath