* [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols @ 2018-08-10 8:06 Andrejs Hanins 2018-08-10 8:06 ` [PATCH 1/1] Calculate inner checksums for all L4 protocols (was for TCP/UDP only) Andrejs Hanins 2018-08-12 8:13 ` [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols Jason A. Donenfeld 0 siblings, 2 replies; 6+ messages in thread From: Andrejs Hanins @ 2018-08-10 8:06 UTC (permalink / raw) To: wireguard 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] Calculate inner checksums for all L4 protocols (was for TCP/UDP only). 2018-08-10 8:06 [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols Andrejs Hanins @ 2018-08-10 8:06 ` Andrejs Hanins 2018-08-12 8:13 ` [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols Jason A. Donenfeld 1 sibling, 0 replies; 6+ messages in thread From: Andrejs Hanins @ 2018-08-10 8:06 UTC (permalink / raw) To: wireguard - 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols 2018-08-10 8:06 [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols Andrejs Hanins 2018-08-10 8:06 ` [PATCH 1/1] Calculate inner checksums for all L4 protocols (was for TCP/UDP only) Andrejs Hanins @ 2018-08-12 8:13 ` Jason A. Donenfeld 2018-08-13 11:11 ` Andrey H. 1 sibling, 1 reply; 6+ messages in thread From: Jason A. Donenfeld @ 2018-08-12 8:13 UTC (permalink / raw) To: ahanins; +Cc: WireGuard mailing list 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols 2018-08-12 8:13 ` [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols Jason A. Donenfeld @ 2018-08-13 11:11 ` Andrey H. 2018-10-18 16:36 ` Jason A. Donenfeld 0 siblings, 1 reply; 6+ messages in thread From: Andrey H. @ 2018-08-13 11:11 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: WireGuard mailing list 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols 2018-08-13 11:11 ` Andrey H. @ 2018-10-18 16:36 ` Jason A. Donenfeld [not found] ` <0d8cad17-593a-9e96-eb4e-dc01c87743fe@gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Jason A. Donenfeld @ 2018-10-18 16:36 UTC (permalink / raw) To: ahanins; +Cc: WireGuard mailing list 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <0d8cad17-593a-9e96-eb4e-dc01c87743fe@gmail.com>]
[parent not found: <be9f1e51-6ed4-3d21-2c20-c7ed6d31aadb@gmail.com>]
* Re: [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols [not found] ` <be9f1e51-6ed4-3d21-2c20-c7ed6d31aadb@gmail.com> @ 2018-10-27 1:23 ` Jason A. Donenfeld 0 siblings, 0 replies; 6+ messages in thread From: Jason A. Donenfeld @ 2018-10-27 1:23 UTC (permalink / raw) To: ahanins; +Cc: WireGuard mailing list 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-27 1:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-10 8:06 [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols Andrejs Hanins 2018-08-10 8:06 ` [PATCH 1/1] Calculate inner checksums for all L4 protocols (was for TCP/UDP only) Andrejs Hanins 2018-08-12 8:13 ` [PATCH 0/1] Fix broken inner checksums for non TCP/UDP protocols Jason A. Donenfeld 2018-08-13 11:11 ` Andrey H. 2018-10-18 16:36 ` Jason A. Donenfeld [not found] ` <0d8cad17-593a-9e96-eb4e-dc01c87743fe@gmail.com> [not found] ` <be9f1e51-6ed4-3d21-2c20-c7ed6d31aadb@gmail.com> 2018-10-27 1:23 ` Jason A. Donenfeld
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).