* [PATCH v2 net 0/3] wireguard fixes for 5.6-rc2 @ 2020-02-14 17:34 Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 1/3] wireguard: selftests: reduce complexity and fix make races Jason A. Donenfeld ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Jason A. Donenfeld @ 2020-02-14 17:34 UTC (permalink / raw) To: davem, netdev; +Cc: Jason A. Donenfeld Here are three fixes for wireguard collected since rc1: 1) Some small cleanups to the test suite to help massively parallel builds. 2) A change in how we reset our load calculation to avoid a more expensive comparison, suggested by Matt Dunwoodie. 3) I've been loading more and more of wireguard's surface into syzkaller, trying to get our coverage as complete as possible, leading in this case to a fix for mtu=0 devices. v2 fixes a logical problem in the patch for (3) pointed out by Eric Dumazet. Jason A. Donenfeld (3): wireguard: selftests: reduce complexity and fix make races wireguard: receive: reset last_under_load to zero wireguard: send: account for mtu=0 devices drivers/net/wireguard/device.c | 7 ++-- drivers/net/wireguard/receive.c | 7 +++- drivers/net/wireguard/send.c | 16 +++++--- .../testing/selftests/wireguard/qemu/Makefile | 38 +++++++------------ 4 files changed, 34 insertions(+), 34 deletions(-) -- 2.25.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 net 1/3] wireguard: selftests: reduce complexity and fix make races 2020-02-14 17:34 [PATCH v2 net 0/3] wireguard fixes for 5.6-rc2 Jason A. Donenfeld @ 2020-02-14 17:34 ` Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 2/3] wireguard: receive: reset last_under_load to zero Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices Jason A. Donenfeld 2 siblings, 0 replies; 12+ messages in thread From: Jason A. Donenfeld @ 2020-02-14 17:34 UTC (permalink / raw) To: davem, netdev; +Cc: Jason A. Donenfeld This gives us fewer dependencies and shortens build time, fixes up some hash checking race conditions, and also fixes missing directory creation that caused issues on massively parallel builds. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- .../testing/selftests/wireguard/qemu/Makefile | 38 +++++++------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/tools/testing/selftests/wireguard/qemu/Makefile b/tools/testing/selftests/wireguard/qemu/Makefile index f10aa3590adc..28d477683e8a 100644 --- a/tools/testing/selftests/wireguard/qemu/Makefile +++ b/tools/testing/selftests/wireguard/qemu/Makefile @@ -38,19 +38,17 @@ endef define file_download = $(DISTFILES_PATH)/$(1): mkdir -p $(DISTFILES_PATH) - flock -x $$@.lock -c '[ -f $$@ ] && exit 0; wget -O $$@.tmp $(MIRROR)$(1) || wget -O $$@.tmp $(2)$(1) || rm -f $$@.tmp' - if echo "$(3) $$@.tmp" | sha256sum -c -; then mv $$@.tmp $$@; else rm -f $$@.tmp; exit 71; fi + flock -x $$@.lock -c '[ -f $$@ ] && exit 0; wget -O $$@.tmp $(MIRROR)$(1) || wget -O $$@.tmp $(2)$(1) || rm -f $$@.tmp; [ -f $$@.tmp ] || exit 1; if echo "$(3) $$@.tmp" | sha256sum -c -; then mv $$@.tmp $$@; else rm -f $$@.tmp; exit 71; fi' endef $(eval $(call tar_download,MUSL,musl,1.1.24,.tar.gz,https://www.musl-libc.org/releases/,1370c9a812b2cf2a7d92802510cca0058cc37e66a7bedd70051f0a34015022a3)) -$(eval $(call tar_download,LIBMNL,libmnl,1.0.4,.tar.bz2,https://www.netfilter.org/projects/libmnl/files/,171f89699f286a5854b72b91d06e8f8e3683064c5901fb09d954a9ab6f551f81)) $(eval $(call tar_download,IPERF,iperf,3.7,.tar.gz,https://downloads.es.net/pub/iperf/,d846040224317caf2f75c843d309a950a7db23f9b44b94688ccbe557d6d1710c)) $(eval $(call tar_download,BASH,bash,5.0,.tar.gz,https://ftp.gnu.org/gnu/bash/,b4a80f2ac66170b2913efbfb9f2594f1f76c7b1afd11f799e22035d63077fb4d)) $(eval $(call tar_download,IPROUTE2,iproute2,5.4.0,.tar.xz,https://www.kernel.org/pub/linux/utils/net/iproute2/,fe97aa60a0d4c5ac830be18937e18dc3400ca713a33a89ad896ff1e3d46086ae)) $(eval $(call tar_download,IPTABLES,iptables,1.8.4,.tar.bz2,https://www.netfilter.org/projects/iptables/files/,993a3a5490a544c2cbf2ef15cf7e7ed21af1845baf228318d5c36ef8827e157c)) $(eval $(call tar_download,NMAP,nmap,7.80,.tar.bz2,https://nmap.org/dist/,fcfa5a0e42099e12e4bf7a68ebe6fde05553383a682e816a7ec9256ab4773faa)) $(eval $(call tar_download,IPUTILS,iputils,s20190709,.tar.gz,https://github.com/iputils/iputils/archive/s20190709.tar.gz/#,a15720dd741d7538dd2645f9f516d193636ae4300ff7dbc8bfca757bf166490a)) -$(eval $(call tar_download,WIREGUARD_TOOLS,wireguard-tools,1.0.20191226,.tar.xz,https://git.zx2c4.com/wireguard-tools/snapshot/,aa8af0fdc9872d369d8c890a84dbc2a2466b55795dccd5b47721b2d97644b04f)) +$(eval $(call tar_download,WIREGUARD_TOOLS,wireguard-tools,1.0.20200206,.tar.xz,https://git.zx2c4.com/wireguard-tools/snapshot/,f5207248c6a3c3e3bfc9ab30b91c1897b00802ed861e1f9faaed873366078c64)) KERNEL_BUILD_PATH := $(BUILD_PATH)/kernel$(if $(findstring yes,$(DEBUG_KERNEL)),-debug) rwildcard=$(foreach d,$(wildcard $1*),$(call rwildcard,$d/,$2) $(filter $(subst *,%,$2),$d)) @@ -295,21 +293,13 @@ $(IPERF_PATH)/src/iperf3: | $(IPERF_PATH)/.installed $(USERSPACE_DEPS) $(MAKE) -C $(IPERF_PATH) $(STRIP) -s $@ -$(LIBMNL_PATH)/.installed: $(LIBMNL_TAR) - flock -s $<.lock tar -C $(BUILD_PATH) -xf $< - touch $@ - -$(LIBMNL_PATH)/src/.libs/libmnl.a: | $(LIBMNL_PATH)/.installed $(USERSPACE_DEPS) - cd $(LIBMNL_PATH) && ./configure --prefix=/ $(CROSS_COMPILE_FLAG) --enable-static --disable-shared - $(MAKE) -C $(LIBMNL_PATH) - sed -i 's:prefix=.*:prefix=$(LIBMNL_PATH):' $(LIBMNL_PATH)/libmnl.pc - $(WIREGUARD_TOOLS_PATH)/.installed: $(WIREGUARD_TOOLS_TAR) + mkdir -p $(BUILD_PATH) flock -s $<.lock tar -C $(BUILD_PATH) -xf $< touch $@ -$(WIREGUARD_TOOLS_PATH)/src/wg: | $(WIREGUARD_TOOLS_PATH)/.installed $(LIBMNL_PATH)/src/.libs/libmnl.a $(USERSPACE_DEPS) - LDFLAGS="$(LDFLAGS) -L$(LIBMNL_PATH)/src/.libs" $(MAKE) -C $(WIREGUARD_TOOLS_PATH)/src LIBMNL_CFLAGS="-I$(LIBMNL_PATH)/include" LIBMNL_LDLIBS="-lmnl" wg +$(WIREGUARD_TOOLS_PATH)/src/wg: | $(WIREGUARD_TOOLS_PATH)/.installed $(USERSPACE_DEPS) + $(MAKE) -C $(WIREGUARD_TOOLS_PATH)/src wg $(STRIP) -s $@ $(BUILD_PATH)/init: init.c | $(USERSPACE_DEPS) @@ -340,17 +330,17 @@ $(BASH_PATH)/bash: | $(BASH_PATH)/.installed $(USERSPACE_DEPS) $(IPROUTE2_PATH)/.installed: $(IPROUTE2_TAR) mkdir -p $(BUILD_PATH) flock -s $<.lock tar -C $(BUILD_PATH) -xf $< - printf 'CC:=$(CC)\nPKG_CONFIG:=pkg-config\nTC_CONFIG_XT:=n\nTC_CONFIG_ATM:=n\nTC_CONFIG_IPSET:=n\nIP_CONFIG_SETNS:=y\nHAVE_ELF:=n\nHAVE_MNL:=y\nHAVE_BERKELEY_DB:=n\nHAVE_LATEX:=n\nHAVE_PDFLATEX:=n\nCFLAGS+=-DHAVE_SETNS -DHAVE_LIBMNL -I$(LIBMNL_PATH)/include\nLDLIBS+=-lmnl' > $(IPROUTE2_PATH)/config.mk + printf 'CC:=$(CC)\nPKG_CONFIG:=pkg-config\nTC_CONFIG_XT:=n\nTC_CONFIG_ATM:=n\nTC_CONFIG_IPSET:=n\nIP_CONFIG_SETNS:=y\nHAVE_ELF:=n\nHAVE_MNL:=n\nHAVE_BERKELEY_DB:=n\nHAVE_LATEX:=n\nHAVE_PDFLATEX:=n\nCFLAGS+=-DHAVE_SETNS\n' > $(IPROUTE2_PATH)/config.mk printf 'lib: snapshot\n\t$$(MAKE) -C lib\nip/ip: lib\n\t$$(MAKE) -C ip ip\nmisc/ss: lib\n\t$$(MAKE) -C misc ss\n' >> $(IPROUTE2_PATH)/Makefile touch $@ -$(IPROUTE2_PATH)/ip/ip: | $(IPROUTE2_PATH)/.installed $(LIBMNL_PATH)/src/.libs/libmnl.a $(USERSPACE_DEPS) - LDFLAGS="$(LDFLAGS) -L$(LIBMNL_PATH)/src/.libs" PKG_CONFIG_LIBDIR="$(LIBMNL_PATH)" $(MAKE) -C $(IPROUTE2_PATH) PREFIX=/ ip/ip - $(STRIP) -s $(IPROUTE2_PATH)/ip/ip +$(IPROUTE2_PATH)/ip/ip: | $(IPROUTE2_PATH)/.installed $(USERSPACE_DEPS) + $(MAKE) -C $(IPROUTE2_PATH) PREFIX=/ ip/ip + $(STRIP) -s $@ -$(IPROUTE2_PATH)/misc/ss: | $(IPROUTE2_PATH)/.installed $(LIBMNL_PATH)/src/.libs/libmnl.a $(USERSPACE_DEPS) - LDFLAGS="$(LDFLAGS) -L$(LIBMNL_PATH)/src/.libs" PKG_CONFIG_LIBDIR="$(LIBMNL_PATH)" $(MAKE) -C $(IPROUTE2_PATH) PREFIX=/ misc/ss - $(STRIP) -s $(IPROUTE2_PATH)/misc/ss +$(IPROUTE2_PATH)/misc/ss: | $(IPROUTE2_PATH)/.installed $(USERSPACE_DEPS) + $(MAKE) -C $(IPROUTE2_PATH) PREFIX=/ misc/ss + $(STRIP) -s $@ $(IPTABLES_PATH)/.installed: $(IPTABLES_TAR) mkdir -p $(BUILD_PATH) @@ -358,8 +348,8 @@ $(IPTABLES_PATH)/.installed: $(IPTABLES_TAR) sed -i -e "/nfnetlink=[01]/s:=[01]:=0:" -e "/nfconntrack=[01]/s:=[01]:=0:" $(IPTABLES_PATH)/configure touch $@ -$(IPTABLES_PATH)/iptables/xtables-legacy-multi: | $(IPTABLES_PATH)/.installed $(LIBMNL_PATH)/src/.libs/libmnl.a $(USERSPACE_DEPS) - cd $(IPTABLES_PATH) && PKG_CONFIG_LIBDIR="$(LIBMNL_PATH)" ./configure --prefix=/ $(CROSS_COMPILE_FLAG) --enable-static --disable-shared --disable-nftables --disable-bpf-compiler --disable-nfsynproxy --disable-libipq --with-kernel=$(BUILD_PATH)/include +$(IPTABLES_PATH)/iptables/xtables-legacy-multi: | $(IPTABLES_PATH)/.installed $(USERSPACE_DEPS) + cd $(IPTABLES_PATH) && ./configure --prefix=/ $(CROSS_COMPILE_FLAG) --enable-static --disable-shared --disable-nftables --disable-bpf-compiler --disable-nfsynproxy --disable-libipq --disable-connlabel --with-kernel=$(BUILD_PATH)/include $(MAKE) -C $(IPTABLES_PATH) $(STRIP) -s $@ -- 2.25.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 net 2/3] wireguard: receive: reset last_under_load to zero 2020-02-14 17:34 [PATCH v2 net 0/3] wireguard fixes for 5.6-rc2 Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 1/3] wireguard: selftests: reduce complexity and fix make races Jason A. Donenfeld @ 2020-02-14 17:34 ` Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices Jason A. Donenfeld 2 siblings, 0 replies; 12+ messages in thread From: Jason A. Donenfeld @ 2020-02-14 17:34 UTC (permalink / raw) To: davem, netdev; +Cc: Jason A. Donenfeld, Matt Dunwoodie This is a small optimization that prevents more expensive comparisons from happening when they are no longer necessary, by clearing the last_under_load variable whenever we wind up in a state where we were under load but we no longer are. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Suggested-by: Matt Dunwoodie <ncon@noconroy.net> --- drivers/net/wireguard/receive.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireguard/receive.c b/drivers/net/wireguard/receive.c index 9c6bab9c981f..4a153894cee2 100644 --- a/drivers/net/wireguard/receive.c +++ b/drivers/net/wireguard/receive.c @@ -118,10 +118,13 @@ static void wg_receive_handshake_packet(struct wg_device *wg, under_load = skb_queue_len(&wg->incoming_handshakes) >= MAX_QUEUED_INCOMING_HANDSHAKES / 8; - if (under_load) + if (under_load) { last_under_load = ktime_get_coarse_boottime_ns(); - else if (last_under_load) + } else if (last_under_load) { under_load = !wg_birthdate_has_expired(last_under_load, 1); + if (!under_load) + last_under_load = 0; + } mac_state = wg_cookie_validate_packet(&wg->cookie_checker, skb, under_load); if ((under_load && mac_state == VALID_MAC_WITH_COOKIE) || -- 2.25.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 17:34 [PATCH v2 net 0/3] wireguard fixes for 5.6-rc2 Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 1/3] wireguard: selftests: reduce complexity and fix make races Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 2/3] wireguard: receive: reset last_under_load to zero Jason A. Donenfeld @ 2020-02-14 17:34 ` Jason A. Donenfeld 2020-02-14 17:56 ` Eric Dumazet 2 siblings, 1 reply; 12+ messages in thread From: Jason A. Donenfeld @ 2020-02-14 17:34 UTC (permalink / raw) To: davem, netdev; +Cc: Jason A. Donenfeld, Eric Dumazet It turns out there's an easy way to get packets queued up while still having an MTU of zero, and that's via persistent keep alive. This commit makes sure that in whatever condition, we don't wind up dividing by zero. Note that an MTU of zero for a wireguard interface is something quasi-valid, so I don't think the correct fix is to limit it via min_mtu. This can be reproduced easily with: ip link add wg0 type wireguard ip link add wg1 type wireguard ip link set wg0 up mtu 0 ip link set wg1 up wg set wg0 private-key <(wg genkey) wg set wg1 listen-port 1 private-key <(wg genkey) peer $(wg show wg0 public-key) wg set wg0 peer $(wg show wg1 public-key) persistent-keepalive 1 endpoint 127.0.0.1:1 However, while min_mtu=0 seems fine, it makes sense to restrict the max_mtu. This commit also restricts the maximum MTU to the greatest number for which rounding up to the padding multiple won't overflow a signed integer. Packets this large were always rejected anyway eventually, due to checks deeper in, but it seems more sound not to even let the administrator configure something that won't work anyway. We use this opportunity to clean up this function a bit so that it's clear which paths we're expecting. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Cc: Eric Dumazet <edumazet@google.com> --- drivers/net/wireguard/device.c | 7 ++++--- drivers/net/wireguard/send.c | 16 +++++++++++----- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c index 43db442b1373..cdc96968b0f4 100644 --- a/drivers/net/wireguard/device.c +++ b/drivers/net/wireguard/device.c @@ -258,6 +258,8 @@ static void wg_setup(struct net_device *dev) enum { WG_NETDEV_FEATURES = NETIF_F_HW_CSUM | NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_GSO | NETIF_F_GSO_SOFTWARE | NETIF_F_HIGHDMA }; + const int overhead = MESSAGE_MINIMUM_LENGTH + sizeof(struct udphdr) + + max(sizeof(struct ipv6hdr), sizeof(struct iphdr)); dev->netdev_ops = &netdev_ops; dev->hard_header_len = 0; @@ -271,9 +273,8 @@ static void wg_setup(struct net_device *dev) dev->features |= WG_NETDEV_FEATURES; dev->hw_features |= WG_NETDEV_FEATURES; dev->hw_enc_features |= WG_NETDEV_FEATURES; - dev->mtu = ETH_DATA_LEN - MESSAGE_MINIMUM_LENGTH - - sizeof(struct udphdr) - - max(sizeof(struct ipv6hdr), sizeof(struct iphdr)); + dev->mtu = ETH_DATA_LEN - overhead; + dev->max_mtu = round_down(INT_MAX, MESSAGE_PADDING_MULTIPLE) - overhead; SET_NETDEV_DEVTYPE(dev, &device_type); diff --git a/drivers/net/wireguard/send.c b/drivers/net/wireguard/send.c index c13260563446..2a9990ab66cd 100644 --- a/drivers/net/wireguard/send.c +++ b/drivers/net/wireguard/send.c @@ -143,16 +143,22 @@ static void keep_key_fresh(struct wg_peer *peer) static unsigned int calculate_skb_padding(struct sk_buff *skb) { + unsigned int padded_size, last_unit = skb->len; + + if (unlikely(!PACKET_CB(skb)->mtu)) + return -last_unit % MESSAGE_PADDING_MULTIPLE; + /* We do this modulo business with the MTU, just in case the networking * layer gives us a packet that's bigger than the MTU. In that case, we * wouldn't want the final subtraction to overflow in the case of the - * padded_size being clamped. + * padded_size being clamped. Fortunately, that's very rarely the case, + * so we optimize for that not happening. */ - unsigned int last_unit = skb->len % PACKET_CB(skb)->mtu; - unsigned int padded_size = ALIGN(last_unit, MESSAGE_PADDING_MULTIPLE); + if (unlikely(last_unit > PACKET_CB(skb)->mtu)) + last_unit %= PACKET_CB(skb)->mtu; - if (padded_size > PACKET_CB(skb)->mtu) - padded_size = PACKET_CB(skb)->mtu; + padded_size = min(PACKET_CB(skb)->mtu, + ALIGN(last_unit, MESSAGE_PADDING_MULTIPLE)); return padded_size - last_unit; } -- 2.25.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 17:34 ` [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices Jason A. Donenfeld @ 2020-02-14 17:56 ` Eric Dumazet 2020-02-14 18:15 ` Jason A. Donenfeld 0 siblings, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2020-02-14 17:56 UTC (permalink / raw) To: Jason A. Donenfeld, davem, netdev; +Cc: Eric Dumazet On 2/14/20 9:34 AM, Jason A. Donenfeld wrote: > It turns out there's an easy way to get packets queued up while still > having an MTU of zero, and that's via persistent keep alive. This commit > makes sure that in whatever condition, we don't wind up dividing by > zero. Note that an MTU of zero for a wireguard interface is something > quasi-valid, so I don't think the correct fix is to limit it via > min_mtu. This can be reproduced easily with: > > ip link add wg0 type wireguard > ip link add wg1 type wireguard > ip link set wg0 up mtu 0 > ip link set wg1 up > wg set wg0 private-key <(wg genkey) > wg set wg1 listen-port 1 private-key <(wg genkey) peer $(wg show wg0 public-key) > wg set wg0 peer $(wg show wg1 public-key) persistent-keepalive 1 endpoint 127.0.0.1:1 > > However, while min_mtu=0 seems fine, it makes sense to restrict the > max_mtu. This commit also restricts the maximum MTU to the greatest > number for which rounding up to the padding multiple won't overflow a > signed integer. Packets this large were always rejected anyway > eventually, due to checks deeper in, but it seems more sound not to even > let the administrator configure something that won't work anyway. > > We use this opportunity to clean up this function a bit so that it's > clear which paths we're expecting. > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > Cc: Eric Dumazet <edumazet@google.com> > --- > drivers/net/wireguard/device.c | 7 ++++--- > drivers/net/wireguard/send.c | 16 +++++++++++----- > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c > index 43db442b1373..cdc96968b0f4 100644 > --- a/drivers/net/wireguard/device.c > +++ b/drivers/net/wireguard/device.c > @@ -258,6 +258,8 @@ static void wg_setup(struct net_device *dev) > enum { WG_NETDEV_FEATURES = NETIF_F_HW_CSUM | NETIF_F_RXCSUM | > NETIF_F_SG | NETIF_F_GSO | > NETIF_F_GSO_SOFTWARE | NETIF_F_HIGHDMA }; > + const int overhead = MESSAGE_MINIMUM_LENGTH + sizeof(struct udphdr) + > + max(sizeof(struct ipv6hdr), sizeof(struct iphdr)); > > dev->netdev_ops = &netdev_ops; > dev->hard_header_len = 0; > @@ -271,9 +273,8 @@ static void wg_setup(struct net_device *dev) > dev->features |= WG_NETDEV_FEATURES; > dev->hw_features |= WG_NETDEV_FEATURES; > dev->hw_enc_features |= WG_NETDEV_FEATURES; > - dev->mtu = ETH_DATA_LEN - MESSAGE_MINIMUM_LENGTH - > - sizeof(struct udphdr) - > - max(sizeof(struct ipv6hdr), sizeof(struct iphdr)); > + dev->mtu = ETH_DATA_LEN - overhead; > + dev->max_mtu = round_down(INT_MAX, MESSAGE_PADDING_MULTIPLE) - overhead; > > SET_NETDEV_DEVTYPE(dev, &device_type); > > diff --git a/drivers/net/wireguard/send.c b/drivers/net/wireguard/send.c > index c13260563446..2a9990ab66cd 100644 > --- a/drivers/net/wireguard/send.c > +++ b/drivers/net/wireguard/send.c > @@ -143,16 +143,22 @@ static void keep_key_fresh(struct wg_peer *peer) > > static unsigned int calculate_skb_padding(struct sk_buff *skb) > { > + unsigned int padded_size, last_unit = skb->len; > + > + if (unlikely(!PACKET_CB(skb)->mtu)) > + return -last_unit % MESSAGE_PADDING_MULTIPLE; My brain hurts. > + > /* We do this modulo business with the MTU, just in case the networking > * layer gives us a packet that's bigger than the MTU. In that case, we > * wouldn't want the final subtraction to overflow in the case of the > - * padded_size being clamped. > + * padded_size being clamped. Fortunately, that's very rarely the case, > + * so we optimize for that not happening. > */ > - unsigned int last_unit = skb->len % PACKET_CB(skb)->mtu; > - unsigned int padded_size = ALIGN(last_unit, MESSAGE_PADDING_MULTIPLE); > + if (unlikely(last_unit > PACKET_CB(skb)->mtu)) > + last_unit %= PACKET_CB(skb)->mtu; > > - if (padded_size > PACKET_CB(skb)->mtu) > - padded_size = PACKET_CB(skb)->mtu; > + padded_size = min(PACKET_CB(skb)->mtu, > + ALIGN(last_unit, MESSAGE_PADDING_MULTIPLE)); > return padded_size - last_unit; > } > Oh dear, can you describe what do you expect of a wireguard device with mtu == 0 or mtu == 1 Why simply not allowing silly configurations, instead of convoluted tests in fast path ? We are speaking of tunnels adding quite a lot of headers, so we better not try to make them work on networks with tiny mtu. Just say no to syzbot. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 17:56 ` Eric Dumazet @ 2020-02-14 18:15 ` Jason A. Donenfeld 2020-02-14 18:22 ` Eric Dumazet 0 siblings, 1 reply; 12+ messages in thread From: Jason A. Donenfeld @ 2020-02-14 18:15 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, Netdev, Eric Dumazet On Fri, Feb 14, 2020 at 6:56 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > Oh dear, can you describe what do you expect of a wireguard device with mtu == 0 or mtu == 1 > > Why simply not allowing silly configurations, instead of convoluted tests in fast path ? > > We are speaking of tunnels adding quite a lot of headers, so we better not try to make them > work on networks with tiny mtu. Just say no to syzbot. The idea was that wireguard might still be useful for the persistent keepalive stuff. This branch becomes very cold very fast, so I don't think it makes a difference performance wise, but if you feel strongly about it, I can get rid of it and set a non-zero min_mtu that's the smallest thing wireguard's xmit semantics will accept. It sounds like you'd prefer that? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 18:15 ` Jason A. Donenfeld @ 2020-02-14 18:22 ` Eric Dumazet 2020-02-14 18:37 ` Jason A. Donenfeld 0 siblings, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2020-02-14 18:22 UTC (permalink / raw) To: Jason A. Donenfeld, Eric Dumazet; +Cc: David Miller, Netdev, Eric Dumazet On 2/14/20 10:15 AM, Jason A. Donenfeld wrote: > On Fri, Feb 14, 2020 at 6:56 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: >> Oh dear, can you describe what do you expect of a wireguard device with mtu == 0 or mtu == 1 >> >> Why simply not allowing silly configurations, instead of convoluted tests in fast path ? >> >> We are speaking of tunnels adding quite a lot of headers, so we better not try to make them >> work on networks with tiny mtu. Just say no to syzbot. > > The idea was that wireguard might still be useful for the persistent > keepalive stuff. This branch becomes very cold very fast, so I don't > think it makes a difference performance wise, but if you feel strongly > about it, I can get rid of it and set a non-zero min_mtu that's the > smallest thing wireguard's xmit semantics will accept. It sounds like > you'd prefer that? > Well, if you believe that wireguard in persistent keepalive has some value on its own, I guess that we will have to support this mode. Some legacy devices can have arbitrary mtu, and this has caused headaches. I was hoping that for brand new devices, we could have saner limits. About setting max_mtu to ~MAX_INT, does it mean wireguard will attempt to send UDP datagrams bigger than 64K ? Where is the segmentation done ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 18:22 ` Eric Dumazet @ 2020-02-14 18:37 ` Jason A. Donenfeld 2020-02-14 18:53 ` Eric Dumazet 0 siblings, 1 reply; 12+ messages in thread From: Jason A. Donenfeld @ 2020-02-14 18:37 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, Netdev, Eric Dumazet On 2/14/20, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > On 2/14/20 10:15 AM, Jason A. Donenfeld wrote: >> On Fri, Feb 14, 2020 at 6:56 PM Eric Dumazet <eric.dumazet@gmail.com> >> wrote: >>> Oh dear, can you describe what do you expect of a wireguard device with >>> mtu == 0 or mtu == 1 >>> >>> Why simply not allowing silly configurations, instead of convoluted tests >>> in fast path ? >>> >>> We are speaking of tunnels adding quite a lot of headers, so we better >>> not try to make them >>> work on networks with tiny mtu. Just say no to syzbot. >> >> The idea was that wireguard might still be useful for the persistent >> keepalive stuff. This branch becomes very cold very fast, so I don't >> think it makes a difference performance wise, but if you feel strongly >> about it, I can get rid of it and set a non-zero min_mtu that's the >> smallest thing wireguard's xmit semantics will accept. It sounds like >> you'd prefer that? >> > Well, if you believe that wireguard in persistent keepalive > has some value on its own, I guess that we will have to support this mode. Alright. > > Some legacy devices can have arbitrary mtu, and this has caused headaches. > I was hoping that for brand new devices, we could have saner limits. > > About setting max_mtu to ~MAX_INT, does it mean wireguard will attempt > to send UDP datagrams bigger than 64K ? Where is the segmentation done ? The before passings off to the udp tunnel api, we indicate that we support ip segmentation, and then it gets handled and fragmented deeper down. Check out socket.c. This winds up being sometimes useful for some odd people when it's faster to encrypt longer packets on networks with no loss. I can't say I generally recommend people go that route, but some report benefitting from it. > -- Jason A. Donenfeld Deep Space Explorer fr: +33 6 51 90 82 66 us: +1 513 476 1200 www.jasondonenfeld.com www.zx2c4.com zx2c4.com/keys/AB9942E6D4A4CFC3412620A749FC7012A5DE03AE.asc ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 18:37 ` Jason A. Donenfeld @ 2020-02-14 18:53 ` Eric Dumazet 2020-02-14 21:57 ` Jason A. Donenfeld 0 siblings, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2020-02-14 18:53 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: David Miller, Netdev, Eric Dumazet On 2/14/20 10:37 AM, Jason A. Donenfeld wrote: > On 2/14/20, Eric Dumazet <eric.dumazet@gmail.com> wrote: >> >> >> On 2/14/20 10:15 AM, Jason A. Donenfeld wrote: >>> On Fri, Feb 14, 2020 at 6:56 PM Eric Dumazet <eric.dumazet@gmail.com> >>> wrote: >>>> Oh dear, can you describe what do you expect of a wireguard device with >>>> mtu == 0 or mtu == 1 >>>> >>>> Why simply not allowing silly configurations, instead of convoluted tests >>>> in fast path ? >>>> >>>> We are speaking of tunnels adding quite a lot of headers, so we better >>>> not try to make them >>>> work on networks with tiny mtu. Just say no to syzbot. >>> >>> The idea was that wireguard might still be useful for the persistent >>> keepalive stuff. This branch becomes very cold very fast, so I don't >>> think it makes a difference performance wise, but if you feel strongly >>> about it, I can get rid of it and set a non-zero min_mtu that's the >>> smallest thing wireguard's xmit semantics will accept. It sounds like >>> you'd prefer that? >>> >> Well, if you believe that wireguard in persistent keepalive >> has some value on its own, I guess that we will have to support this mode. > > Alright. > >> >> Some legacy devices can have arbitrary mtu, and this has caused headaches. >> I was hoping that for brand new devices, we could have saner limits. >> >> About setting max_mtu to ~MAX_INT, does it mean wireguard will attempt >> to send UDP datagrams bigger than 64K ? Where is the segmentation done ? > > The before passings off to the udp tunnel api, we indicate that we > support ip segmentation, and then it gets handled and fragmented > deeper down. Check out socket.c. Okay. Speaking of socket.c, I found this wg_socket_reinit() snippet : synchronize_rcu(); synchronize_net(); Which makes little sense. Please add a comment explaining why these two calls are needed. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 18:53 ` Eric Dumazet @ 2020-02-14 21:57 ` Jason A. Donenfeld 2020-02-14 22:30 ` Eric Dumazet 0 siblings, 1 reply; 12+ messages in thread From: Jason A. Donenfeld @ 2020-02-14 21:57 UTC (permalink / raw) To: Eric Dumazet; +Cc: Netdev Hey Eric, On Fri, Feb 14, 2020 at 7:53 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > The before passings off to the udp tunnel api, we indicate that we > > support ip segmentation, and then it gets handled and fragmented > > deeper down. Check out socket.c. > > Okay. Speaking of socket.c, I found this wg_socket_reinit() snippet : > > synchronize_rcu(); > synchronize_net(); > > Which makes little sense. Please add a comment explaining why these two > calls are needed. Thanks, I appreciate your scrutiny here. Right again, you are. It looks like that was added in 2017 after observing the pattern in other drivers and seeing the documentation comment, "Wait for packets currently being received to be done." That sounds like an important thing to do before tearing down a socket. But here it makes no sense at all, since synchronize_net() is just a wrapper around synchronize_rcu() (and sometimes _expedited). And here, the synchronize_rcu() usage makes sense to have, since this is as boring of an rcu pattern as can be: mutex_lock() old = rcu_dereference_protected(x->y) rcu_assign(x->y, new) mutex_unlock() synchronize_rcu() free_it(old) Straight out of the documentation. Having the extra synchronize_net() in there adds nothing at all. I'll send a v3 of this 5.6-rc2 cleanup series containing that removal. Jason ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 21:57 ` Jason A. Donenfeld @ 2020-02-14 22:30 ` Eric Dumazet 2020-02-14 22:53 ` Jason A. Donenfeld 0 siblings, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2020-02-14 22:30 UTC (permalink / raw) To: Jason A. Donenfeld, Eric Dumazet; +Cc: Netdev On 2/14/20 1:57 PM, Jason A. Donenfeld wrote: > > Thanks, I appreciate your scrutiny here. Right again, you are. It > looks like that was added in 2017 after observing the pattern in other > drivers and seeing the documentation comment, "Wait for packets > currently being received to be done." That sounds like an important > thing to do before tearing down a socket. But here it makes no sense > at all, since synchronize_net() is just a wrapper around > synchronize_rcu() (and sometimes _expedited). And here, the > synchronize_rcu() usage makes sense to have, since this is as boring > of an rcu pattern as can be: > > mutex_lock() > old = rcu_dereference_protected(x->y) > rcu_assign(x->y, new) > mutex_unlock() > synchronize_rcu() > free_it(old) > > Straight out of the documentation. Having the extra synchronize_net() > in there adds nothing at all. I'll send a v3 of this 5.6-rc2 cleanup > series containing that removal. > Also note that UDP sockets have SOCK_RCU_FREE flag set, so core networking also respect one RCU grace period before freeing them. It is possible that no extra synchronize_{net|rcu}() call is needed, but this is left as an exercise for future kernels :) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices 2020-02-14 22:30 ` Eric Dumazet @ 2020-02-14 22:53 ` Jason A. Donenfeld 0 siblings, 0 replies; 12+ messages in thread From: Jason A. Donenfeld @ 2020-02-14 22:53 UTC (permalink / raw) To: Eric Dumazet; +Cc: Netdev On Fri, Feb 14, 2020 at 11:30 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > Also note that UDP sockets have SOCK_RCU_FREE flag set, so core > networking also respect one RCU grace period before freeing them. if (use_call_rcu) call_rcu(&sk->sk_rcu, __sk_destruct); else __sk_destruct(&sk->sk_rcu); Ah, that's handy indeed. > It is possible that no extra synchronize_{net|rcu}() call is needed, > but this is left as an exercise for future kernels :) Cool, yea, sounds like something I should play with for 5.7. Sending v3 out in a few minutes. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-02-14 22:53 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-14 17:34 [PATCH v2 net 0/3] wireguard fixes for 5.6-rc2 Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 1/3] wireguard: selftests: reduce complexity and fix make races Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 2/3] wireguard: receive: reset last_under_load to zero Jason A. Donenfeld 2020-02-14 17:34 ` [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices Jason A. Donenfeld 2020-02-14 17:56 ` Eric Dumazet 2020-02-14 18:15 ` Jason A. Donenfeld 2020-02-14 18:22 ` Eric Dumazet 2020-02-14 18:37 ` Jason A. Donenfeld 2020-02-14 18:53 ` Eric Dumazet 2020-02-14 21:57 ` Jason A. Donenfeld 2020-02-14 22:30 ` Eric Dumazet 2020-02-14 22:53 ` 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).