On Tue, 30 Jun 2020 09:22:18 +1200 (NZST) 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? Depends on your definition of significant, I've seen 1-3% throughput improvement without the patch. Without patch: --- 10.0.0.2 tcpbench statistics --- 4455088992 bytes sent over 121.001 seconds bandwidth min/avg/max/std-dev = 122.940/294.737/324.080/30.782 Mbps With patch: --- 10.0.0.2 tcpbench statistics --- 4366145736 bytes sent over 120.999 seconds bandwidth min/avg/max/std-dev = 123.908/288.718/329.770/31.518 Mbps > 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. Overall, it is still debatable whether to skip the IPv4 checksum as modern crypto certainly offers better integrity checks. However, the primary motivator for skipping the integrity checks is performance, and the performance isn't severely impacted. Additionally, I can sympathise with avoiding layer violations and bringing it inline with other tunnels in this case. I'm happy for the following patch to be committed. - Matt > 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. > > 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;
