diff --git a/MAINTAINERS b/MAINTAINERS index 23e9194483d9..7636ab50d44f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20537,6 +20537,7 @@ F: drivers/irqchip/irq-or1k-* OPENVPN DATA CHANNEL OFFLOAD M: Antonio Quartulli +R: Ralf Lici R: Sabrina Dubroca L: openvpn-devel@lists.sourceforge.net (subscribers-only) L: netdev@vger.kernel.org diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c index e42b60cd04a9..62a5ccfd3502 100644 --- a/drivers/net/ovpn/bind.c +++ b/drivers/net/ovpn/bind.c @@ -18,7 +18,7 @@ * ovpn_bind_from_sockaddr - retrieve binding matching sockaddr * @ss: the sockaddr to match * - * Return: the bind matching the passed sockaddr if found, NULL otherwise + * Return: the new bind for the passed sockaddr, an ERR_PTR() on failure */ struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *ss) { diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c index 9526f8096da6..f14eb4e6c46a 100644 --- a/drivers/net/ovpn/io.c +++ b/drivers/net/ovpn/io.c @@ -206,8 +206,7 @@ void ovpn_decrypt_post(void *data, int ret) drop_nocount: if (likely(ks)) ovpn_crypto_key_slot_put(ks); - if (likely(peer)) - ovpn_peer_put(peer); + ovpn_peer_put(peer); } /* RX path entry point: decrypt packet and forward it to the device */ @@ -305,8 +304,7 @@ void ovpn_encrypt_post(void *data, int ret) kfree_skb(skb); if (likely(ks)) ovpn_crypto_key_slot_put(ks); - if (likely(peer)) - ovpn_peer_put(peer); + ovpn_peer_put(peer); } static bool ovpn_encrypt_one(struct ovpn_peer *peer, struct sk_buff *skb) diff --git a/drivers/net/ovpn/socket.h b/drivers/net/ovpn/socket.h index 4afcec71040d..5c87392ecd30 100644 --- a/drivers/net/ovpn/socket.h +++ b/drivers/net/ovpn/socket.h @@ -24,7 +24,6 @@ struct ovpn_peer; * @peer: unique peer transmitting over this socket (TCP only) * @sk: the low level sock object * @refcount: amount of contexts currently referencing this object - * @work: member used to schedule release routine (it may block) * @tcp_tx_work: work for deferring outgoing packet processing (TCP only) */ struct ovpn_socket { @@ -38,7 +37,6 @@ struct ovpn_socket { struct sock *sk; struct kref refcount; - struct work_struct work; struct work_struct tcp_tx_work; }; diff --git a/tools/testing/selftests/net/ovpn/Makefile b/tools/testing/selftests/net/ovpn/Makefile index 169f0464ac3a..35e4c9d4fc68 100644 --- a/tools/testing/selftests/net/ovpn/Makefile +++ b/tools/testing/selftests/net/ovpn/Makefile @@ -42,6 +42,7 @@ TEST_PROGS := \ test-symmetric-id-float.sh \ test-symmetric-id-tcp.sh \ test-symmetric-id.sh \ + test-tcp-nagle.sh \ test-tcp.sh \ test.sh \ # end of TEST_PROGS diff --git a/tools/testing/selftests/net/ovpn/common.sh b/tools/testing/selftests/net/ovpn/common.sh index 5e9c81e885e6..da732c64ffea 100644 --- a/tools/testing/selftests/net/ovpn/common.sh +++ b/tools/testing/selftests/net/ovpn/common.sh @@ -191,20 +191,14 @@ ovpn_setup_ns() { ovpn_build_capture_filter() { # match the first four bytes of the openvpn data payload - if [ "${OVPN_PROTO}" == "UDP" ]; then - # For UDP, libpcap transport indexing only works for IPv4, so - # use an explicit IPv4 or IPv6 expression based on the peer - # address. The IPv6 branch assumes there are no extension - # headers in the outer packet. - if [[ "${2}" == *:* ]]; then - printf "ip6 and ip6[6] = 17 and ip6[48:4] = %s" "${1}" - else - printf "ip and udp[8:4] = %s" "${1}" - fi + # For UDP, libpcap transport indexing only works for IPv4, so + # use an explicit IPv4 or IPv6 expression based on the peer + # address. The IPv6 branch assumes there are no extension + # headers in the outer packet. + if [[ "${2}" == *:* ]]; then + printf "ip6 and ip6[6] = 17 and ip6[48:4] = %s" "${1}" else - # openvpn over TCP prepends a 2-byte packet length ahead of the - # DATA_V2 opcode, so skip it before matching the payload header - printf "ip and tcp[(((tcp[12] & 0xf0) >> 2) + 2):4] = %s" "${1}" + printf "ip and udp[8:4] = %s" "${1}" fi } @@ -262,14 +256,19 @@ ovpn_add_peer() { fi else if [ ${1} -eq 0 ]; then + OVPN_TCP_KEYS_LOG=$(mktemp) (ip netns exec "${server_ns}" ${OVPN_CLI} listen tun0 \ 1 ${M_ID} ${OVPN_TCP_PEERS_FILE} && { + rc=0 for p in $(seq 1 ${OVPN_NUM_PEERS}); do ip netns exec "${server_ns}" \ ${OVPN_CLI} new_key tun0 ${p} \ - 1 0 ${OVPN_ALG} 0 data64.key + 1 0 ${OVPN_ALG} 0 data64.key \ + || rc=1 done - }) & + exit ${rc} + }) >"${OVPN_TCP_KEYS_LOG}" 2>&1 & + OVPN_TCP_KEYS_PID=$! sleep 5 else peer_ns="ovpn_peer${1}" @@ -287,6 +286,58 @@ ovpn_add_peer() { fi } +# In TCP mode the server accepts the peers and installs their data keys from a +# background subshell. "listen" returns once a connection has been accepted for +# every entry of the peers file, so the key installation runs after the last +# ovpn_add_peer() call. Wait for that subshell before generating any traffic, +# otherwise the first packets may race the key installation and get dropped for +# lack of a key. +# +# The wait is bounded: both the accept loop and the subsequent receive in +# ovpn-cli are unbounded, so a peer that never connects (for instance when +# OVPN_NUM_PEERS is smaller than the number of entries in the peers file) would +# otherwise hang the whole test instead of failing. +OVPN_TCP_KEYS_TIMEOUT=${OVPN_TCP_KEYS_TIMEOUT:-30} + +ovpn_wait_tcp_keys() { + local deadline rc + + [ -n "${OVPN_TCP_KEYS_PID:-}" ] || return 0 + + deadline=$((SECONDS + OVPN_TCP_KEYS_TIMEOUT)) + while kill -0 "${OVPN_TCP_KEYS_PID}" 2>/dev/null; do + if [ "${SECONDS}" -ge "${deadline}" ]; then + printf '%s\n' \ + "server-side key setup still running after ${OVPN_TCP_KEYS_TIMEOUT}s" + kill "${OVPN_TCP_KEYS_PID}" 2>/dev/null + wait "${OVPN_TCP_KEYS_PID}" 2>/dev/null + ovpn_dump_tcp_keys_log + return 1 + fi + sleep 0.2 + done + + wait "${OVPN_TCP_KEYS_PID}" + rc=$? + + # ovpn_cmd_run() has already removed the temporary file this subshell + # inherited, so without this its output would be lost and a failed + # new_key would only show up later as a ping failure. + if [ "${rc}" -ne 0 ]; then + ovpn_dump_tcp_keys_log + fi + rm -f "${OVPN_TCP_KEYS_LOG}" + + return "${rc}" +} + +ovpn_dump_tcp_keys_log() { + [ -s "${OVPN_TCP_KEYS_LOG:-}" ] || return 0 + + printf '%s\n' "--- server-side key setup output ---" + cat "${OVPN_TCP_KEYS_LOG}" +} + ovpn_compare_ntfs() { local diff_rc=0 local diff_file diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c index 3b612a8a18fe..00b80bc5157f 100644 --- a/tools/testing/selftests/net/ovpn/ovpn-cli.c +++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -16,6 +17,7 @@ #include #include #include +#include #include #include @@ -470,6 +472,33 @@ static int ovpn_parse_key_direction(const char *dir, struct ovpn_ctx *ctx) return 0; } +/* openvpn 2.7.6 and later always set TCP_NODELAY, so mirror that by + * default. Peers running 2.7.0 to 2.7.5 still leave Nagle enabled, and + * mixed-version deployments will keep doing so for a while, so let the + * selftests exercise that configuration too by setting OVPN_TCP_NODELAY=0. + */ +static bool ovpn_tcp_nodelay_wanted(void) +{ + const char *env = getenv("OVPN_TCP_NODELAY"); + + return !env || strcmp(env, "0"); +} + +static int ovpn_tcp_nodelay(int socket) +{ + int opt = 1; + int ret; + + if (!ovpn_tcp_nodelay_wanted()) + return 0; + + ret = setsockopt(socket, IPPROTO_TCP, TCP_NODELAY, &opt, sizeof(opt)); + if (ret < 0) + perror("setsockopt for TCP_NODELAY"); + + return ret; +} + static int ovpn_socket(struct ovpn_ctx *ctx, sa_family_t family, int proto) { struct sockaddr_storage local_sock = { 0 }; @@ -606,6 +635,12 @@ static int ovpn_accept(struct ovpn_ctx *ctx) goto err; } + if (ovpn_tcp_nodelay(ret) < 0) { + close(ret); + ret = -1; + goto err; + } + return ret; err: close(ctx->socket); @@ -623,6 +658,10 @@ static int ovpn_connect(struct ovpn_ctx *ovpn) return -1; } + ret = ovpn_tcp_nodelay(s); + if (ret < 0) + goto err; + switch (ovpn->remote.in4.sin_family) { case AF_INET: socklen = sizeof(struct sockaddr_in); diff --git a/tools/testing/selftests/net/ovpn/test-close-socket.sh b/tools/testing/selftests/net/ovpn/test-close-socket.sh index ec9a51bbf3c9..142dc14e2606 100755 --- a/tools/testing/selftests/net/ovpn/test-close-socket.sh +++ b/tools/testing/selftests/net/ovpn/test-close-socket.sh @@ -37,6 +37,8 @@ ovpn_prepare_network() { ovpn_cmd_ok "register peer${p} in overlay" ovpn_add_peer "${p}" done + ovpn_cmd_ok "wait for server-side key setup" ovpn_wait_tcp_keys + for p in $(seq 1 ${OVPN_NUM_PEERS}); do peer_ns="ovpn_peer${p}" ovpn_cmd_ok "set peer0 timeout for peer ${p}" \ diff --git a/tools/testing/selftests/net/ovpn/test-tcp-nagle.sh b/tools/testing/selftests/net/ovpn/test-tcp-nagle.sh new file mode 100755 index 000000000000..2d94d8e14d3d --- /dev/null +++ b/tools/testing/selftests/net/ovpn/test-tcp-nagle.sh @@ -0,0 +1,12 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2025 OpenVPN, Inc. +# +# Author: Antonio Quartulli + +# Peers running openvpn 2.7.0 to 2.7.5 leave Nagle enabled on their TCP +# sockets. Keep exercising that configuration so it does not silently break. +OVPN_PROTO="TCP" +export OVPN_TCP_NODELAY=0 + +source test.sh diff --git a/tools/testing/selftests/net/ovpn/test.sh b/tools/testing/selftests/net/ovpn/test.sh index 392109d5e14e..0762fa83e4b5 100755 --- a/tools/testing/selftests/net/ovpn/test.sh +++ b/tools/testing/selftests/net/ovpn/test.sh @@ -45,6 +45,8 @@ ovpn_prepare_network() { ovpn_cmd_ok "register peer${p} in overlay" ovpn_add_peer "${p}" done + ovpn_cmd_ok "wait for server-side key setup" ovpn_wait_tcp_keys + for p in $(seq 1 ${OVPN_NUM_PEERS}); do peer_ns="ovpn_peer${p}" ovpn_cmd_ok "set peer0 timeout for peer ${p}" \ @@ -137,35 +139,33 @@ ovpn_run_basic_traffic() { local tcpdump_timeout="1.5s" for p in $(seq 1 ${OVPN_NUM_PEERS}); do - # The first part of the data packet header consists of: - # - TCP only: 2 bytes for the packet length - # - 5 bits for opcode ("9" for DATA_V2) - # - 3 bits for key-id ("0" at this point) - # - 12 bytes for peer-id: - # - with asymmetric ID: "${p}" one way and "${p} + 9" the - # other way - # - with symmetric ID: "${p}" both ways - header1=$(printf "0x4800000%x" ${p}) - header2=$(printf "0x4800000%x" $((p + OVPN_ID_OFFSET))) - raddr="" if [ "${OVPN_PROTO}" == "UDP" ]; then + # The first part of the data packet header consists of: + # - 5 bits for opcode ("9" for DATA_V2) + # - 3 bits for key-id ("0" at this point) + # - 3 bytes for peer-id: + # - with asymmetric ID: "${p}" one way and "${p} + 9" the + # other way + # - with symmetric ID: "${p}" both ways + header1=$(printf "0x4800000%x" ${p}) + header2=$(printf "0x4800000%x" $((p + OVPN_ID_OFFSET))) raddr=$(awk "NR == ${p} {print \$3}" \ "${OVPN_UDP_PEERS_FILE}") + peer_ns="ovpn_peer${p}" + + timeout ${tcpdump_timeout} ip netns exec "${peer_ns}" \ + tcpdump --immediate-mode -p -ni veth${p} -c 1 \ + "$(ovpn_build_capture_filter "${header1}" "${raddr}")" \ + >/dev/null 2>&1 & + tcpdump_pid1=$! + timeout ${tcpdump_timeout} ip netns exec "${peer_ns}" \ + tcpdump --immediate-mode -p -ni veth${p} -c 1 \ + "$(ovpn_build_capture_filter "${header2}" "${raddr}")" \ + >/dev/null 2>&1 & + tcpdump_pid2=$! + + sleep 0.3 fi - peer_ns="ovpn_peer${p}" - - timeout ${tcpdump_timeout} ip netns exec "${peer_ns}" \ - tcpdump --immediate-mode -p -ni veth${p} -c 1 \ - "$(ovpn_build_capture_filter "${header1}" "${raddr}")" \ - >/dev/null 2>&1 & - tcpdump_pid1=$! - timeout ${tcpdump_timeout} ip netns exec "${peer_ns}" \ - tcpdump --immediate-mode -p -ni veth${p} -c 1 \ - "$(ovpn_build_capture_filter "${header2}" "${raddr}")" \ - >/dev/null 2>&1 & - tcpdump_pid2=$! - - sleep 0.3 ovpn_cmd_ok "send baseline traffic to peer ${p}" \ ip netns exec ovpn_peer0 \ ping -qfc 100 -w 3 5.5.5.$((p + 1)) @@ -173,8 +173,10 @@ ovpn_run_basic_traffic() { ip netns exec ovpn_peer0 \ ping -qfc 100 -s 3000 -w 3 5.5.5.$((p + 1)) - wait "${tcpdump_pid1}" || return 1 - wait "${tcpdump_pid2}" || return 1 + if [ "${OVPN_PROTO}" == "UDP" ]; then + wait "${tcpdump_pid1}" || return 1 + wait "${tcpdump_pid2}" || return 1 + fi done }