From owner-freebsd-current@FreeBSD.ORG Wed Nov 12 15:13:19 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6253416A4DD for ; Wed, 12 Nov 2003 15:13:19 -0800 (PST) Received: from mailtoaster1.pipeline.ch (mailtoaster1.pipeline.ch [62.48.0.70]) by mx1.FreeBSD.org (Postfix) with ESMTP id B5AE343FDF for ; Wed, 12 Nov 2003 15:13:15 -0800 (PST) (envelope-from oppermann@pipeline.ch) Received: (qmail 33232 invoked from network); 12 Nov 2003 23:16:07 -0000 Received: from unknown (HELO pipeline.ch) ([62.48.0.54]) (envelope-sender ) by mailtoaster1.pipeline.ch (qmail-ldap-1.03) with SMTP for ; 12 Nov 2003 23:16:07 -0000 Message-ID: <3FB2BE8A.6C880085@pipeline.ch> Date: Thu, 13 Nov 2003 00:13:14 +0100 From: Andre Oppermann X-Mailer: Mozilla 4.76 [en] (Windows NT 5.0; U) X-Accept-Language: en MIME-Version: 1.0 To: Jesper Skriver References: <3FAE68FB.64D262FF@pipeline.ch> <20031112225326.GI41949@FreeBSD.org> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit cc: freebsd-net@freebsd.org cc: freebsd-current@freebsd.org cc: mb@imp.ch cc: ume@freebsd.org cc: sam@errno.com Subject: Re: tcp hostcache and ip fastforward for review X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Nov 2003 23:13:19 -0000 Jesper Skriver wrote: > > On Sun, Nov 09, 2003 at 05:19:07PM +0100, Andre Oppermann wrote: > > Hello all, > > > > this patch contains three things (to be separated for committing): ... > > ip_fastforward > > > > - removes ip_flow forwarding code > > - adds full direct process-to-completion IPv4 forwarding code > > - handles ip fragmentation incl. hw support (ip_flow did not) > > - supports ipfw and ipfilter (ip_flow did not) > > - supports divert and ipfw fwd (ip_flow did not) > > - drops anything it can't handle back to normal ip_input > > I have a few comments to this code, see inline, look for #jesper Answers also inline. [All whitespace bugs are fixed and omitted here] > Apart from that it looks good. Thanks for reviewing! > /Jesper > > > +int > > +ip_fastforward(struct mbuf *m) > > +{ ... > > + > > + /* > > + * Only unicast IP, not from loopback, no L2 or IP broadcast, > > + * no multicast, no INADDR_ANY > > + */ > > + if ((m->m_pkthdr.rcvif->if_flags & IFF_LOOPBACK) || > > + (ntohl(ip->ip_src.s_addr) == (u_long)INADDR_BROADCAST) || > > #jesper > You will never see packets with a multicast source address. I hope so but we can never be sure. Here we look at what we've got straight from the wire. Everything is possible there. I only need to craft an apropriate packet... > > + (ntohl(ip->ip_dst.s_addr) == (u_long)INADDR_BROADCAST) || > > + (IN_MULTICAST(ntohl(ip->ip_src.s_addr))) || > > + (IN_MULTICAST(ntohl(ip->ip_dst.s_addr))) || > > + (ip->ip_dst.s_addr == INADDR_ANY) ) > > + goto fallback; ... > > + /* > > + * Or is it for a local IP broadcast address on this host? > > + */ > > + if (m->m_pkthdr.rcvif->if_flags & IFF_BROADCAST) { > > + TAILQ_FOREACH(ifa, &m->m_pkthdr.rcvif->if_addrhead, ifa_link) { > > + if (ifa->ifa_addr->sa_family != AF_INET) > > + continue; > > + ia = ifatoia(ifa); > > + if (ia->ia_netbroadcast.s_addr == ip->ip_dst.s_addr) > > + goto fallback; > > + if (satosin(&ia->ia_broadaddr)->sin_addr.s_addr == > > + ip->ip_dst.s_addr) > > + goto fallback; > > + continue; > > +fallback: > > + /* drop the packet back to netisr */ > > + ip->ip_len = htons(ip->ip_len); > > + ip->ip_off = htons(ip->ip_off); > > + return 0; > > + } > > + } > > + ipstat.ips_total++; > > #jesper > If we stored special "for us" /32 routes in the routing table for > addresses configured on this host, we could avoid the above 2 loops, > which can quite expensive. Good idea. I will look at that after 5.2 code freeze. > These special routes will simply mean that the packet is for us, and > needs to given to ip_input > > > + /** > > + ** Third: incoming packet firewall processing > > + **/ > > + > > + odest = dest = ip->ip_dst.s_addr; > > #jesper > You could save a few cycles by doing Well, you're right. However I don't think it makes any difference and I'd like to keep it the current way for clarity and ease of reading and understanding the code. > #ifdef PFIL_HOOKS > odest = ip->ip_dst.s_addr; > /* > * Run through list of ipfilter hooks for input packets > */ > if (pfil_run_hooks(&inet_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN) || > m == NULL) > return 1; > > M_ASSERTVALID(m); > M_ASSERTPKTHDR(m); > > ip = mtod(m, struct ip *); /* if m changed during fw processing */ > dest = ip->ip_dst.s_addr; > #else > odest = dest = ip->ip_dst.s_addr; > #endif > > Thus avoiding writing to dest twice. > > > +#ifdef PFIL_HOOKS > > + /* > > + * Run through list of ipfilter hooks for input packets > > + */ > > + if (pfil_run_hooks(&inet_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN) || > > + m == NULL) > > + return 1; > > + > > + M_ASSERTVALID(m); > > + M_ASSERTPKTHDR(m); > > + > > + ip = mtod(m, struct ip *); /* if m changed during fw processing */ > > + dest = ip->ip_dst.s_addr; > > +#endif ... > > +passin: > > + ip = mtod(m, struct ip *); /* if m changed during fw processing */ > > + > > + /* > > + * Destination address changed? > > + */ > > + if (odest != dest) { > > + /* > > + * Is it now for a local address on this host? > > + */ > > + LIST_FOREACH(ia, INADDR_HASH(ip->ip_dst.s_addr), ia_hash) { > > + if (IA_SIN(ia)->sin_addr.s_addr == ip->ip_dst.s_addr) > > + goto forwardlocal; > > + } > > #jesper > > Same comment as above - and do we really want to see if the original > destination address was ours if we're doing NAT ? Yes, we have to do that for ipfw fwd and ipfilter address rewrite (from ipnat). It may be that the packet has been hijacked in a transparent proxy situation and has to be delivered to a local socket. > > + /* > > + * Go on with new destination address > > + */ ... > > + /* > > + * Destination address changed? > > + */ > > + if (odest != dest) { > > + /* > > + * Is it now for a local address on this host? > > + */ > > #jesper > > Again, do we really want to look for packets destined for us after being > translated ? Same answer as above. > > + LIST_FOREACH(ia, INADDR_HASH(ip->ip_dst.s_addr), ia_hash) { > > + if (IA_SIN(ia)->sin_addr.s_addr == ip->ip_dst.s_addr) { > > +forwardlocal: ... -- Andre