On Wed, Nov 14, 2018 at 08:16:33PM +1000, David Gwynne wrote: > On Wed, Nov 14, 2018 at 11:49:02AM +1000, David Gwynne wrote: > > while looking at outgoing ttl processing and rfc 6040, i noted that gif > > and gre patched a packets ttl, but didn't update the checksum on the > > packet. > > > > i think there's two issues here. firstly, we need to update the checksum > > when the packet is patched, but only for v4, and secondly, we should > > clear the csum_flags on an mbuf after the first decap. > > > > this patch addresses the first issue. i have removed the ttl patching in > > gre and gif, but gif and the ipip code still patch the ecn on a packet. > > gif did not update the v4 checksum, and ipip recalculates the whole > > checksum. > > > > the stuff procter worked on in pf shows that you can do a small > > update to the checksum rather than recalcuate the whole thing. this > > adds a function to the ecn code that does just that for tos field, > > which is where the ecn is store. > > > > it also updates gif and ipip to use it. > > > > ok? > > i forgot to say that having ipip code blindly recalculate the checksum > means that a faulty inner packet will look fine when it shouldnt. it is > much better to update the checksum with the ecn change, and let the > whole cksum check fail later on. > > this is a better diff. this one even compiles. > > Index: netinet/ip_ecn.c > =================================================================== > RCS file: /cvs/src/sys/netinet/ip_ecn.c,v > retrieving revision 1.8 > diff -u -p -r1.8 ip_ecn.c > --- netinet/ip_ecn.c 24 Sep 2016 14:51:37 -0000 1.8 > +++ netinet/ip_ecn.c 14 Nov 2018 10:13:14 -0000 > @@ -154,3 +154,24 @@ ip_ecn_egress(int mode, u_int8_t *outer, > } > return (1); > } > + > +/* > + * Patch the checksum with the difference between the old and new tos. > + * The patching is based on what pf_patch_8() and pf_cksum_fixkup() do, > + * but they're in pf, so we can't rely on them being available. > + */ > +void > +ip_tos_patch(struct ip *ip, uint8_t tos) > +{ > + uint16_t old; > + uint16_t new; > + uint32_t x; > + > + old = htons(ip->ip_tos); > + new = htons(tos); > + > + ip->ip_tos = tos; > + > + x = ip->ip_sum + old - new; > + ip->ip_sum = (x) + (x >> 16); > +} > Index: netinet/ip_ecn.h > =================================================================== > RCS file: /cvs/src/sys/netinet/ip_ecn.h,v > retrieving revision 1.6 > diff -u -p -r1.6 ip_ecn.h > --- netinet/ip_ecn.h 15 Mar 2012 16:37:11 -0000 1.6 > +++ netinet/ip_ecn.h 14 Nov 2018 10:13:14 -0000 > @@ -47,5 +47,6 @@ > #if defined(_KERNEL) > extern void ip_ecn_ingress(int, u_int8_t *, u_int8_t *); > extern int ip_ecn_egress(int, u_int8_t *, u_int8_t *); > +void ip_tos_patch(struct ip *, uint8_t); > #endif /* _KERNEL */ > #endif /* _NETINET_IP_ECN_H_ */ > Index: netinet/ip_ipip.c > =================================================================== > RCS file: /cvs/src/sys/netinet/ip_ipip.c,v > retrieving revision 1.88 > diff -u -p -r1.88 ip_ipip.c > --- netinet/ip_ipip.c 28 Aug 2018 15:15:02 -0000 1.88 > +++ netinet/ip_ipip.c 14 Nov 2018 10:13:14 -0000 > @@ -239,16 +239,14 @@ ipip_input_if(struct mbuf **mp, int *off > itos = ip->ip_tos; > mode = m->m_flags & (M_AUTH|M_CONF) ? > ECN_ALLOWED_IPSEC : ECN_ALLOWED; > - if (!ip_ecn_egress(mode, &otos, &ip->ip_tos)) { > + if (!ip_ecn_egress(mode, &otos, &itos)) { > DPRINTF(("%s: ip_ecn_egress() failed\n", __func__)); > ipipstat_inc(ipips_pdrops); > goto bad; > } > /* re-calculate the checksum if ip_tos was changed */ > - if (itos != ip->ip_tos) { > - ip->ip_sum = 0; > - ip->ip_sum = in_cksum(m, hlen); > - } > + if (itos != ip->ip_tos) > + ip_tos_patch(ip, itos); > break; > #ifdef INET6 > case IPPROTO_IPV6: > Index: net/if_gif.c > =================================================================== > RCS file: /cvs/src/sys/net/if_gif.c,v > retrieving revision 1.123 > diff -u -p -r1.123 if_gif.c > --- net/if_gif.c 14 Nov 2018 03:20:03 -0000 1.123 > +++ net/if_gif.c 14 Nov 2018 10:13:14 -0000 > @@ -809,7 +809,8 @@ gif_input(struct gif_tunnel *key, struct > if (ip_ecn_egress(ECN_ALLOWED, &otos, &itos) == 0) > goto drop; > > - ip->ip_tos = itos; > + if (itos != ip->ip_tos) > + ip_tos_patch(ip, itos); > > m->m_pkthdr.ph_family = AF_INET; >
Reads good to me. OK claudio@ -- :wq Claudio