Re: tcp hostcache and ip fastforward for review

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