[PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols

[PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols

[Andrejs Hanins]

Hi,
    I'm using GRE tunnel (transparent Ethernet bridging flavor) over Wireguard
interface to be able to bridge L2 network segments. The typical protocol chain
looks like this IP->GRE->EthernetHeader->IP->UDP. UDP here is the packet sent
from the L2 network segment which is tunneled using GRE over Wireguard. Indeed,
there is a checksum inside UDP header which is, as a rule, kept partially
calculated while packet travels through network stack and outer protocols are
added until the packet reaches WG device which exposes NETIF_F_HW_CSUM feature
meaning it can handle checksum offload for all protocols.

    But the problem here is that skb_checksum_setup called from skb_encrypt
handles only TCP/UDP protocols under top level IP, but in my case there is a
GRE protocol there, so skb_checksum_help is not called and packet continues its
life with unfinished (broken) checksum and gets encrypted as-is. When such
packet is received by other side and reaches L2 networks it's seen there with
a broken checksum inside the UDP header.

    The fact that Wireguard on the receiving side sets skb->ip_summed to
CHECKSUM_UNNECESSARY partially mitigates the problem by telling network stack
on the receiving side that validation of the checksum is not necessary, so
local TCP stack, for example, works fine. But it doesn't help in situations
when packet needs to be forwarded further (sent out from the box). In this case
there is no way we can tell next hop that checksum verification for this packet
is not necessary, we just send it out with bad checksum and packet gets dropped
on the next hop box.

    I think the issue of the original code was the wrong usage of
skb_checksum_setup, simply because it's not needed in this case. Instead, we
can just rely on ip_summed skb field to see if partial checksum needs to be
finalized or not. Note that many other drivers in kernel follow this approach.

    I'm not a kernel networking expert, but additional change (paranoid?)
could be to call skb_checksum_setup followed by skb_checksum_help for packets
which don't have CHECKSUM_PARTIAL in order to make sure we handle some buggy
packets. This trick is done in checksum_setup from xen-netback. But I'm
really not sure whether it's needed in Wireguard case...

____________
BR, Andrey

Andrejs Hanins (1):
  Calculate inner checksums for all L4 protocols (was for TCP/UDP only).

 src/send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/1] Calculate inner checksums for all L4 protocols (was for TCP/UDP only).

[Andrejs Hanins]

- skb_checksum_setup can only handle TCP/UDP protocols under top level
IP header, packets with other protocols (like GRE) are sent out by
Wireguard with unfinished partial checksums which causes problems on
receiving side (bad checksums).

- skb_encrypt gets skb prepared by network stack, so there is no need to
setup the checksum from scratch, but just perform hw checksum offload
using software helper skb_checksum_help for packet which explicitly
require it as denoted by CHECKSUM_PARTIAL.

Signed-off-by: Andrejs Hanins <ahanins@gmail.com>
---
 src/send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/send.c b/src/send.c
index 3af7ef3..1d021ae 100644
--- a/src/send.c
+++ b/src/send.c
@@ -151,9 +151,9 @@ static inline bool skb_encrypt(struct sk_buff *skb, struct noise_keypair *keypai
 	if (unlikely(skb_cow_head(skb, DATA_PACKET_HEAD_ROOM) < 0))
 		return false;
 
-	/* We have to remember to add the checksum to the innerpacket, in case the receiver forwards it. */
-	if (likely(!skb_checksum_setup(skb, true)))
-		skb_checksum_help(skb);
+	/* Finalize checksum calculation for the inner packet, if required. */
+	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
+		return false;
 
 	/* Only after checksumming can we safely add on the padding at the end and the header. */
 	skb_set_inner_network_header(skb, 0);
-- 
2.17.1

Re: [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols

[Jason A. Donenfeld]

Hey Andrejs,

Thanks a lot for this patch and for the nice write-up. I'm still not
sure, though, that I totally understand the checksumming semantics
properly. I see that this basic pattern is used in tap.c and in a few
other random drivers, but different places seem to do it differently
from that too.

Your paranoid proposal at the end of your description would be
something like this?

> if (skb->ip_summed == CHECKSUM_PARTIAL || !skb_checksum_setup(skb, true)) {
>    if (skb_checksum_help(skb))
>        return false;
> }

And the bug you're pointing out is that skb_checksum_setup returns
-EPROTO in the case of GRE and such, because a child function,
skb_checksum_setup_ip, only works for UDP/TCP?

Do you know why there exist these two separate functions in the first
place, and what the preferred use cases are for each one? Also, do you
know about the relative performance of them and how your patch or the
paranoid variant above would impact that?

Thanks,
Jason

Re: [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols

[Andrey H.]

Hi Jason,

On 08/12/2018 11:13 AM, Jason A. Donenfeld wrote:

> Thanks a lot for this patch and for the nice write-up. I'm still not
> sure, though, that I totally understand the checksumming semantics
> properly.
Neither do I :) But skbuff.h gives quite clear instructions about
CHECKSUM_PARTIAL semantics and handling for outgoing packets, namely:

...the driver MUST ensure that the checksum is set correctly. A driver 
can either offload the checksum calculation to the device, or call 
skb_checksum_help (in the case that the device does not support offload 
for a particular checksum).

So the usage of skb_checksum_help is mandatory, because Wireguard device 
itself doesn't have any HW offload capabilities, obviously. But anyway, 
WG device must advertise NETIF_F_HW_CSUM capability, otherwise 
networking stack will not feed GSO super packets to it, that's what I 
have understood while analyzing the root cause of this checksum bug.

As expected, the bug can also be fixed by removing NETIF_F_HW_CSUM bit 
from dev features, but it disables GSO, so not good.

> 
> Your paranoid proposal at the end of your description would be
> something like this?
> 
>> if (skb->ip_summed == CHECKSUM_PARTIAL || !skb_checksum_setup(skb, true)) {
>>     if (skb_checksum_help(skb))
>>         return false;
>> }
> 
> And the bug you're pointing out is that skb_checksum_setup returns
> -EPROTO in the case of GRE and such, because a child function,
> skb_checksum_setup_ip, only works for UDP/TCP?
> 
Yes and yes.

> Do you know why there exist these two separate functions in the first
> place, and what the preferred use cases are for each one?
If you are referring to skb_checksum_help and skb_checksum_setup, then 
they serve different purposes. skb_checksum_help being a supper widely 
used function to finalize partial checksum calculation in case there is 
no HW offload caps (exactly WG case, as it's a virtual device), whereas 
skb_checksum_setup is a function which basically re-calculates checksum 
offset inside skb based on known protocols (only TCP/UDP!) and 
optionally re-calculates the checksum of pseudo-headers which _is_ 
somewhat costly operation and original code does request it by calling 
skb_checksum_setup(skb, true).

So for me it doesn't make sense to call this heavy "setup" function for 
an skb which came directly from the networking stack and we known it's 
been properly setup.

> Also, do you
> know about the relative performance of them and how your patch or the
> paranoid variant above would impact that?
My patch should only increase the performance, because it eliminates 
unnecessary re-initialization of partial checksum and pseudo-header 
summation.

I've just realized that paranoid version could even violate the rules. 
Imagine, there is an skb with ip_summed _not_ equal to CHECKSUM_PARTIAL, 
in this case we would try to forcibly setup partial checksum for this 
packet and finalize it, which doesn't sound right to me, because other 
ip_summed values don't require the driver to tinker with checksum.

_____
BR, Andrey

Re: [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols

[Jason A. Donenfeld]

Hi Andrey,

Circling back to this now. In light of the questions I raised, do you
think I should proceed with this patch, or are you reworking?

Jason
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols

[Jason A. Donenfeld]

Merged as https://git.zx2c4.com/WireGuard/commit/?id=d985b8c767ed8a20d9cf1c368f153e6a52299aaf

Thanks for your analysis and sorry for the delay.

Regards,
Jason
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard