On Tue, Jun 30, 2020 at 09:22:18AM +1200, richard.n.proc...@gmail.com wrote: > Hi Jason, > > On 27/06/2020, at 10:09 PM, Jason A. Donenfeld <ja...@zx2c4.com> wrote: > > [...] I thought I'd lay out my understanding of the matter, > > and you can let me know whether or not this corresponds with what you > > had in mind: > > [...] > > My main concern is the end-to-end TCP or UDP transport checksum of the > tunneled (or inner, or encapsulated) packet. Your argument seems to > concern instead the per-hop IPv4 header checksum (which is also > interesting to look at, but first things first). > > As I read it, wg_decap() tells the stack to ignore the transport checksum > of the tunneled packet, and I think this is incorrect for the following > reason: the packet may have been corrupted outside of the tunnel, because > the tunnel needn't cover the entire path the packet took through the > network. > > So a tunneled packet may be corrupt, and one addressed to the tunnel > endpoint will be delivered with its end-to-end transport checksum > unverified. Higher layers may receive corrupt data as as result.[*] > > (Routers, as intermediate network elements, are under no obligation to > check end-to-end transport checksums. It isn't their job.) > > One might argue (I do not) that this isn't a concern, because encryption > these days is ubiquitous and comes with far stronger integrity checks. > Nonetheless, even here transport checksums remain relevant for two > reasons. Firsly, because networks remain unreliable: I see non-zero > numbers of failed checksums on my systems. And, secondly, because higher > layers require a reliable transport: TLS for instance will drop the > connection when the MAC check fails[1]. > > Let's turn to the per-hop IPv4 checksum, which wg_decap() also tells the > stack to ignore. I suspect the cost of verifying that checksum -- 20 bytes > on a hot cache line -- is negligible both absolutely and relative to the > cost of decrypting the packet. And as the optimisation appears nowhere > else in the tree that I can find, removing it would make wg(4) no worse > than the other tunnels. Do you have evidence that the cost is in fact > significant? > > Further, as Theo pointed out, the link layer has in any case no business > with the IPv4 checksum, being part of the network layer. Layer violations > are problematic for at least two reasons. Firstly, they constrict the > design space. For instance, I suppose the IPv4 checksum needn't be > per-hop. It might be updated incrementally, increasing its scope. But this > would be nullified by any link layer that, acting on a false assumption > about the layer above it, told the stack to ignore that layer's checksum. > Secondly, layer violations, an optimisation, which almost by definition > rely on additional premises, burden the correctness obligation and so > trade less work for the computer in narrow circumstances for (often much) > more work for the fallible human to show that the process remains sound. > > On that balance I now think that skipping the IPv4 check a false economy, > too. Hopefully I have managed to make my reasons clearer this time around, > please let me know if not, or if you disagree. > > See updated patch below, looking for oks to commit that. > > cheers, > Richard. > > [*] By way of background, I understand the transport checksum was added to > the early ARPANET primarily to guard against corruption inside its > routers[0], after a router fault which "[brought] the network to its > knees". In other words, end-to-end checks are necessary to catch faults in > the routers; link-level checks aren't enough. > > More generally, transport checksums set a lower bound on the reliability > of the end-to-end transport, which decouples it from the reliability of > the underlying network. > > [0] Kleinrock "Queuing Systems, Volume 2: Computer Applications", > John Wiley and Sons (1976), p441 > > [1] RFC5246 TLS v1.2 (2008), section 7.2.2. > > TLS's requirement is reasonable: it would otherwise be obliged to > reimplement for itself retransmission, which would drag with it other > transport layer mechanisms like acknowledgements and (for performance) > sequence numbers and sliding windows.
I agree with all of this and think that this is the right fix. OK claudio@ > Index: net/if_wg.c > =================================================================== > RCS file: /cvs/src/sys/net/if_wg.c,v > retrieving revision 1.7 > diff -u -p -u -p -r1.7 if_wg.c > --- net/if_wg.c 23 Jun 2020 10:03:49 -0000 1.7 > +++ net/if_wg.c 28 Jun 2020 20:17:07 -0000 > @@ -1660,14 +1660,8 @@ wg_decap(struct wg_softc *sc, struct mbu > goto error; > } > > - /* > - * We can mark incoming packet csum OK. We mark all flags OK > - * irrespective to the packet type. > - */ > - m->m_pkthdr.csum_flags |= (M_IPV4_CSUM_IN_OK | M_TCP_CSUM_IN_OK | > - M_UDP_CSUM_IN_OK | M_ICMP_CSUM_IN_OK); > - m->m_pkthdr.csum_flags &= ~(M_IPV4_CSUM_IN_BAD | M_TCP_CSUM_IN_BAD | > - M_UDP_CSUM_IN_BAD | M_ICMP_CSUM_IN_BAD); > + /* tunneled packet was not offloaded */ > + m->m_pkthdr.csum_flags = 0; > > m->m_pkthdr.ph_ifidx = sc->sc_if.if_index; > m->m_pkthdr.ph_rtableid = sc->sc_if.if_rdomain; > -- :wq Claudio
