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
