From owner-svn-src-head@FreeBSD.ORG Thu Apr 25 08:25:01 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 79B1C990; Thu, 25 Apr 2013 08:25:01 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.69.10]) by mx1.freebsd.org (Postfix) with ESMTP id EB8D8153D; Thu, 25 Apr 2013 08:24:59 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.6/8.14.6) with ESMTP id r3P8Owu1068878; Thu, 25 Apr 2013 12:24:58 +0400 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.6/8.14.6/Submit) id r3P8Owfw068877; Thu, 25 Apr 2013 12:24:58 +0400 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Thu, 25 Apr 2013 12:24:58 +0400 From: Gleb Smirnoff To: Randall Stewart Subject: Re: svn commit: r249848 - head/sys/netinet Message-ID: <20130425082458.GG76816@glebius.int.ru> References: <201304241830.r3OIUWiZ073002@svn.freebsd.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="b//ZgE2eAae+kIBt" Content-Disposition: inline In-Reply-To: <201304241830.r3OIUWiZ073002@svn.freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Apr 2013 08:25:01 -0000 --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--