tcp hostcache and ip fastforward for review
Andre Oppermann
oppermann at pipeline.ch
Wed Nov 12 15:13:19 PST 2003
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
More information about the freebsd-current
mailing list