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.
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;
