Date: Thu, 25 Apr 2013 12:24:58 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Randall Stewart <rrs@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r249848 - head/sys/netinet Message-ID: <20130425082458.GG76816@glebius.int.ru> In-Reply-To: <201304241830.r3OIUWiZ073002@svn.freebsd.org> References: <201304241830.r3OIUWiZ073002@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--b//ZgE2eAae+kIBt Content-Type: text/plain; charset=koi8-r Content-Disposition: inline On Wed, Apr 24, 2013 at 06:30:32PM +0000, Randall Stewart wrote: R> Author: rrs R> Date: Wed Apr 24 18:30:32 2013 R> New Revision: 249848 R> URL: http://svnweb.freebsd.org/changeset/base/249848 R> R> Log: R> This fixes the issue with the "randomly changing" default R> route. What it was is there are two places in ip_output.c R> where we do a goto again. One place was fine, it R> copies out the new address and then resets dst = ro->rt_dst; R> But the other place does *not* do that, which means earlier R> when we found the gateway, we have dst pointing there R> aka dst = ro->rt_gateway is done.. then we do a R> goto again.. bam now we clobber the default route. R> R> The fix is just to move the again so we are always R> doing dst = &ro->rt_dst; in the again loop. R> R> PR: 174749,157796 R> MFC after: 1 week This dst pointing either on stack or into routing table is dangerous. We already have several places where the problem is carefully handled, and now you fixed another one. Nevertheless this is subtle and leaves a place for future bugs. I think we should introduce a pointer to const struct sockaddr_in, which either matches dst or rte->rt_gateway. Patch attached. -- Totus tuus, Glebius. --b//ZgE2eAae+kIBt Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="ip_output.c.diff" Index: ip_output.c =================================================================== --- ip_output.c (revision 249884) +++ ip_output.c (working copy) @@ -123,6 +123,7 @@ ip_output(struct mbuf *m, struct mbuf *opt, struct int n; /* scratchpad */ int error = 0; struct sockaddr_in *dst; + const struct sockaddr_in *gw; struct in_ifaddr *ia; int isbroadcast; uint16_t ip_len, ip_off; @@ -196,8 +197,8 @@ ip_output(struct mbuf *m, struct mbuf *opt, struct hlen = ip->ip_hl << 2; } + gw = dst = (struct sockaddr_in *)&ro->ro_dst; again: - dst = (struct sockaddr_in *)&ro->ro_dst; ia = NULL; /* * If there is a cached route, @@ -297,11 +298,11 @@ again: ifp = rte->rt_ifp; rte->rt_rmx.rmx_pksent++; if (rte->rt_flags & RTF_GATEWAY) - dst = (struct sockaddr_in *)rte->rt_gateway; + gw = (struct sockaddr_in *)rte->rt_gateway; if (rte->rt_flags & RTF_HOST) isbroadcast = (rte->rt_flags & RTF_BROADCAST); else - isbroadcast = in_broadcast(dst->sin_addr, ifp); + isbroadcast = in_broadcast(gw->sin_addr, ifp); } /* * Calculate MTU. If we have a route that is up, use that, @@ -327,12 +328,6 @@ again: if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr))) { m->m_flags |= M_MCAST; /* - * IP destination address is multicast. Make sure "dst" - * still points to the address in "ro". (It may have been - * changed to point to a gateway address, above.) - */ - dst = (struct sockaddr_in *)&ro->ro_dst; - /* * See if the caller provided any multicast options */ if (imo != NULL) { @@ -559,7 +554,6 @@ sendit: /* Or forward to some other address? */ if ((m->m_flags & M_IP_NEXTHOP) && (fwd_tag = m_tag_find(m, PACKET_TAG_IPFORWARD, NULL)) != NULL) { - dst = (struct sockaddr_in *)&ro->ro_dst; bcopy((fwd_tag+1), dst, sizeof(struct sockaddr_in)); m->m_flags |= M_SKIP_FIREWALL; m->m_flags &= ~M_IP_NEXTHOP; @@ -628,8 +622,7 @@ passout: * to avoid confusing lower layers. */ m->m_flags &= ~(M_PROTOFLAGS); - error = (*ifp->if_output)(ifp, m, - (struct sockaddr *)dst, ro); + error = (*ifp->if_output)(ifp, m, (struct sockaddr *)gw, ro); goto done; } @@ -663,7 +656,7 @@ passout: m->m_flags &= ~(M_PROTOFLAGS); error = (*ifp->if_output)(ifp, m, - (struct sockaddr *)dst, ro); + (struct sockaddr *)gw, ro); } else m_freem(m); } --b//ZgE2eAae+kIBt--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130425082458.GG76816>