Re: wg(4): encapsulated transport checksums

2020-06-30 Thread Matt Dunwoodie
On Tue, 30 Jun 2020 20:40:10 -0600
"Theo de Raadt"  wrote:

> Matt Dunwoodie  wrote:
> 
> > Depends on your definition of significant, I've seen 1-3% throughput
> > improvement without the patch.  
> 
> > Real networks require statistics, which you want to throw away.  
> 
> > 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.  
> 
> If it is debatable, why don't you debate it?  I don't see a debate.
> 
> Let me debate it.
> 
> The issue is not about integrity checks being needed, but about
> integrity check counters -- such counters are used to short-cut
> procedures during network diagostic failures in multi-configuration
> systems.
> 
> If a higher-level network overlay skips that counter updates for
> lower-levels, the counters are incorrect, now how do you diagnose
> quickly?  Well, you don't.

Before going any further, I should clarify that the outer packet
checksums are already checked (with or without this patch) in
ip_input_if and udp_input when being received on the lower-level
interface. Therefore, any lower-level link layer corruption to the
outer packet will be caught, dropped and counters incremented before
being passed to wg_input. Does that change any of your following points?

> It sounds like the overlay is being chosen and relevant as more
> important than the underlay.  Sorry to burst your bubble, but the
> overlay will never be the whole internet.  The underlay will persist
> for a long time, and the underlay will see errors.  But the counters
> indicating those erors will be *incoherent*.
> 
> To me, it seems your path leads to the inablity to diagnose underlying
> issues correctly and quickly
> 
> Are underlying issues suddenly absent, or rare enough, they don't need
> quick diagnosis?
> 
> Or do (all) overlay technologies now provide enough information
> access to make evaluation of underlying failures easy?
> 
> For those questions, in my experience, I don't think reality provides
> easy paths yet.
> 
> As I said, argue it from a non-wg diagnosis model.  If the argument is
> not convincing, we have to do the obvious right thing, even if it
> costs a small amount.
> 
> Honestly, i don't understand how you ended in the position you are.

Now, the "debatability" is about whether we want to check the inner
packet IPv4 checksum after successful decryption (not about counters).
The story may be different if you have any cases to add to the three
Jason sent through earlier. I said "debatable" because I still think
both sides are vaild, however when I weigh up the arguments I see
applying the patch as the right thing to do.

- Matt



Re: wg(4): encapsulated transport checksums

2020-06-30 Thread Theo de Raadt
Matt Dunwoodie  wrote:

> Depends on your definition of significant, I've seen 1-3% throughput
> improvement without the patch.

> Real networks require statistics, which you want to throw away.

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

If it is debatable, why don't you debate it?  I don't see a debate.

Let me debate it.

The issue is not about integrity checks being needed, but about
integrity check counters -- such counters are used to short-cut
procedures during network diagostic failures in multi-configuration
systems.

If a higher-level network overlay skips that counter updates for
lower-levels, the counters are incorrect, now how do you diagnose
quickly?  Well, you don't.

It sounds like the overlay is being chosen and relevant as more
important than the underlay.  Sorry to burst your bubble, but the
overlay will never be the whole internet.  The underlay will persist for
a long time, and the underlay will see errors.  But the counters
indicating those erors will be *incoherent*.

To me, it seems your path leads to the inablity to diagnose underlying
issues correctly and quickly

Are underlying issues suddenly absent, or rare enough, they don't need
quick diagnosis?

Or do (all) overlay technologies now provide enough information access to
make evaluation of underlying failures easy?

For those questions, in my experience, I don't think reality provides
easy paths yet.

As I said, argue it from a non-wg diagnosis model.  If the argument is
not convincing, we have to do the obvious right thing, even if it costs
a small amount.

Honestly, i don't understand how you ended in the position you are.



Re: wg(4): encapsulated transport checksums

2020-06-30 Thread Matt Dunwoodie
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 
> 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 

Re: wg(4): encapsulated transport checksums

2020-06-30 Thread Claudio Jeker
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  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 -  1.7
> +++ net/if_wg.c   28 Jun 2020 20:17:07 -
> @@ -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 | 

Re: wg(4): encapsulated transport checksums

2020-06-29 Thread richard . n . procter
Hi Jason,

On 27/06/2020, at 10:09 PM, Jason A. Donenfeld  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 -  1.7
+++ net/if_wg.c 28 Jun 2020 20:17:07 -
@@ -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;



Re: wg(4): encapsulated transport checksums

2020-06-27 Thread Theo de Raadt
> - Therefore, it's not necessary to check the IP checksum on ingress because:

There is actually a really good reason.

There are various counters (of all packets) which people observe to debug
network problems.

Now, if lower-level packets carrying wg with corruption don't increment
those counters, the statistics will be incorrect.

I think you are arguying to elide mandatory work in a lower layer of
network stack, isn't it a layer violation to insist like that?



Re: wg(4): encapsulated transport checksums

2020-06-27 Thread Jason A. Donenfeld
Hi Richard,

Thanks for the patch. I had problems parsing some terminology in your
description, so 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:

- On egress, we must compute the packet checksum, because it may well
be forwarded by the receiving end after decapsulation. That doesn't
concern this patch, however.

- On ingress, we've already checked the poly1305 sum, so we have no
doubt that the packet has arrived without corruption.
- Therefore, it's not necessary to check the IP checksum on ingress because:
  * If the packet originated on the peer that did the encapsulation,
there's no chance for corruption;
  * If the packet did not originate on the peer that did the
encapsulation, it was that peer's responsibility to drop it if the
checksum was wrong;
  * If the packet does have an incorrect checksum, because the
originating peer did not check it, and we forward it along, the
machine we forward it to will drop it.

It seemed like from your message that you had a case in mind in which
it actually would be necessary to check the IP checksum on ingress,
but I didn't quite divine what you had in mind.

Jason

On Fri, Jun 26, 2020 at 10:03 PM  wrote:
>
> Hi,
>
> On its receive path, wg(4) uses the same mbuf for both the encrypted
> capsule and its encapsulated packet, which it passes up to the stack. We
> must therefore clear this mbuf's checksum status flags, as although the
> capsule may have been subject to hardware offload, its encapsulated packet
> was not.
>
> This ensures that the transport checksums of packets bound for local
> delivery are verified. That is necessary because, although the tunnel
> provides stronger integrity checks, the tunnel endpoints and the
> transport endpoints needn't coincide.
>
> However, as the network and tunnel endpoints _do_ conincide, it remains
> unncessary to check the per-hop IPv4 checksum.
>
> ok?
>
> 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 -  1.7
> +++ net/if_wg.c 27 Jun 2020 02:48:37 -
> @@ -1660,14 +1660,10 @@ 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;
> +   /* optimise: the tunnel provided a stronger integrity check */
> +   m->m_pkthdr.csum_flags |= M_IPV4_CSUM_IN_OK;
>
> m->m_pkthdr.ph_ifidx = sc->sc_if.if_index;
> m->m_pkthdr.ph_rtableid = sc->sc_if.if_rdomain;