[PATCH net 1/7] ovpn: avoid sending UDP packets with source port 0

[PATCH net 1/7] ovpn: avoid sending UDP packets with source port 0

[Antonio Quartulli]

From: Ralf Lici <ralf@mandelbit.com>

ovpn operates on a userspace-owned UDP socket, which may be manipulated
in various ways by userspace. If the socket is never bound, connected,
or used for communication, it may not have a source port assigned.
Similarly, if the socket was connect()'ed to AF_INET or AF_INET6, it can
be disconnected by connect() with AF_UNSPEC, which resets the source
port unless the socket was explicitly bound.

Since we must not transmit packets with source port 0, gate UDP TX on
the presence of a valid source port and drop packets otherwise. To avoid
ambiguity, sample the current source port once before route lookup and
header build and enforce the check on that value.

Emit a ratelimited warning when this drop path is hit so the broken
socket state is visible. Return local UDP output errors to the common TX
completion path so locally dropped packets are not counted as successful
transport TX and do not refresh the peer keepalive timer.

Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
Cc: stable@vger.kernel.org
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
[antonio: rebased on top of the ovpn_route_key rework: check the
 key.sport snapshot once in ovpn_udp_send_skb()]
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c  |  4 +++-
 drivers/net/ovpn/udp.c | 24 +++++++++++++++++-------
 drivers/net/ovpn/udp.h |  4 ++--
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 9526f8096da6..ce1993b243e6 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -282,7 +282,9 @@ void ovpn_encrypt_post(void *data, int ret)
 
 	switch (sock->sk->sk_protocol) {
 	case IPPROTO_UDP:
-		ovpn_udp_send_skb(peer, sock->sk, skb);
+		ret = ovpn_udp_send_skb(peer, sock->sk, skb);
+		if (unlikely(ret < 0))
+			goto err_unlock;
 		break;
 	case IPPROTO_TCP:
 		ovpn_tcp_send_skb(peer, sock->sk, skb);
diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index 055cdb1bee13..30c7eb261e7b 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -407,7 +407,8 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
  * @key: route key snapshot used for cache validation and flow lookup
  *
  * rcu_read_lock should be held on entry.
- * On return, the skb is consumed.
+ * On success, the skb is passed to the transport stack and consumed. On
+ * error, ownership remains with the caller.
  *
  * Return: 0 on success or a negative error code otherwise
  */
@@ -455,15 +456,26 @@ static int ovpn_udp_output(struct ovpn_peer *peer, struct dst_cache *cache,
  * @peer: the destination peer
  * @sk: peer socket
  * @skb: the packet to send
+ *
+ * On success, the skb is passed to the transport stack and consumed. On
+ * error, ownership remains with the caller.
+ *
+ * Return: 0 on success or a negative error code otherwise
  */
-void ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
-		       struct sk_buff *skb)
+int ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
+		      struct sk_buff *skb)
 {
 	struct ovpn_route_key key = {
 		.mark = READ_ONCE(sk->sk_mark),
 		.sport = READ_ONCE(inet_sk(sk)->inet_sport),
 	};
-	int ret;
+
+	/* an uninitialized socket or connect(AF_UNSPEC) can cause this */
+	if (unlikely(!key.sport)) {
+		net_warn_ratelimited("%s: peer %u: UDP source port is 0\n",
+				     netdev_name(peer->ovpn->dev), peer->id);
+		return -EADDRNOTAVAIL;
+	}
 
 	skb->dev = peer->ovpn->dev;
 	skb->mark = key.mark;
@@ -473,9 +485,7 @@ void ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 	ovpn_dst_cache_check_key(peer, &peer->dst_cache, &key);
 
 	/* crypto layer -> transport (UDP) */
-	ret = ovpn_udp_output(peer, &peer->dst_cache, sk, skb, &key);
-	if (unlikely(ret < 0))
-		kfree_skb(skb);
+	return ovpn_udp_output(peer, &peer->dst_cache, sk, skb, &key);
 }
 
 static void ovpn_udp_encap_destroy(struct sock *sk)
diff --git a/drivers/net/ovpn/udp.h b/drivers/net/ovpn/udp.h
index fe26fbe25c5a..5b67112162a5 100644
--- a/drivers/net/ovpn/udp.h
+++ b/drivers/net/ovpn/udp.h
@@ -19,7 +19,7 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock, struct socket *sock,
 			   struct ovpn_priv *ovpn);
 void ovpn_udp_socket_detach(struct ovpn_socket *ovpn_sock);
 
-void ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
-		       struct sk_buff *skb);
+int ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
+		      struct sk_buff *skb);
 
 #endif /* _NET_OVPN_UDP_H_ */

Re: [PATCH net 1/7] ovpn: avoid sending UDP packets with source port 0

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 1/7] ovpn: avoid sending UDP packets with source port 0

[Antonio Quartulli]

On 22/10/2026 14:23, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>   - How the issue was discovered, e.g. hit in production, hit during
>     development, syzbot report, manual code inspection, LLM or static
>     analysis tool scan.

Patches 1 to 6 are fixes for pre-existing issues reported by Sashiko 
some time ago, that we confirmed after code analysis.

They are "theoretical" because OpenVPN in userspace is not hitting these 
bugs and we have not witnessed these issues in production, but other 
software may.

Patch 7 is fixing a crash in the selftests binary that we actually hit.


Regards,

> 
>   - Whether the issue was actually triggered, or is only theoretical
>     (e.g. found by code inspection). If it was triggered please include
>     the symptoms, like the stack trace or error messages.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

-- 
Antonio Quartulli
OpenVPN Inc.

[PATCH net 2/7] ovpn: validate sockets before attaching peer transports

[Antonio Quartulli]

From: Ralf Lici <ralf@mandelbit.com>

ovpn accepts a userspace-provided socket and attaches transport-specific
state to it. The current checks use sk_protocol to select the UDP or TCP
attach path, but sk_protocol alone does not identify the socket layout.
For example, a raw socket can have sk_protocol set to IPPROTO_UDP while
its storage is not a struct udp_sock. Passing such a socket to the UDP
attach path would make ovpn read and write udp_sock fields on the wrong
object, potentially accessing memory beyond the actual socket storage.

Reject sockets unless they are real UDP datagram or TCP stream sockets
before attaching them to ovpn in the peer creation path. This lets
netlink report a clear error before calling the socket attach helper.

Also switch ovpn_socket_new to sk_is_tcp and sk_is_udp, matching the
netlink validation performed before the helper is called. This does not
change the accepted socket types, but makes the helper's assumptions
explicit.

Fixes: f6226ae7a0cd ("ovpn: introduce the ovpn_socket object")
Fixes: 1d36a36f6d53 ("ovpn: implement peer add/get/dump/delete via netlink")
Cc: stable@vger.kernel.org
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/netlink.c | 15 +++++++++++++--
 drivers/net/ovpn/socket.c  | 16 +++++++++-------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index 5432bc2eb8e8..1af7e0e624ba 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -448,10 +448,21 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 		goto peer_release;
 	}
 
+	/* sk_protocol is not enough to determine if this is a real UDP or TCP
+	 * socket
+	 */
+	if (!sk_is_udp(sock->sk) && !sk_is_tcp(sock->sk)) {
+		NL_SET_ERR_MSG_FMT_MOD(info->extack,
+				       "socket is not TCP or UDP");
+		sockfd_put(sock);
+		ret = -EOPNOTSUPP;
+		goto peer_release;
+	}
+
 	/* Only when using UDP as transport protocol the remote endpoint
 	 * can be configured so that ovpn knows where to send packets to.
 	 */
-	if (sock->sk->sk_protocol == IPPROTO_UDP &&
+	if (sk_is_udp(sock->sk) &&
 	    !attrs[OVPN_A_PEER_REMOTE_IPV4] &&
 	    !attrs[OVPN_A_PEER_REMOTE_IPV6]) {
 		NL_SET_ERR_MSG_FMT_MOD(info->extack,
@@ -465,7 +476,7 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 	 * will just send bytes over it, without the need to specify a
 	 * destination.
 	 */
-	if (sock->sk->sk_protocol == IPPROTO_TCP &&
+	if (sk_is_tcp(sock->sk) &&
 	    (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
 	     attrs[OVPN_A_PEER_REMOTE_IPV6])) {
 		NL_SET_ERR_MSG_FMT_MOD(info->extack,
diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
index 6cbeb2caaeec..4765f4063b71 100644
--- a/drivers/net/ovpn/socket.c
+++ b/drivers/net/ovpn/socket.c
@@ -126,13 +126,15 @@ static int ovpn_socket_attach(struct ovpn_socket *ovpn_sock,
 
 /**
  * ovpn_socket_new - create a new socket and initialize it
- * @sock: the kernel socket to embed
+ * @sock: the kernel socket to embed; must be a real UDP or TCP socket
  * @peer: the peer reachable via this socket
  *
  * Return: an openvpn socket on success or a negative error code otherwise
  */
 struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 {
+	const bool tcp = sk_is_tcp(sock->sk);
+	const bool udp = sk_is_udp(sock->sk);
 	struct ovpn_socket *ovpn_sock;
 	struct sock *sk = sock->sk;
 	int ret;
@@ -142,7 +144,7 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 	/* a TCP socket can only be owned by a single peer, therefore there
 	 * can't be any other user
 	 */
-	if (sk->sk_protocol == IPPROTO_TCP && sk->sk_user_data) {
+	if (tcp && sk->sk_user_data) {
 		ovpn_sock = ERR_PTR(-EBUSY);
 		goto sock_release;
 	}
@@ -150,7 +152,7 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 	/* a UDP socket can be shared across multiple peers, but we must make
 	 * sure it is not owned by something else
 	 */
-	if (sk->sk_protocol == IPPROTO_UDP) {
+	if (udp) {
 		u8 type = READ_ONCE(udp_sk(sk)->encap_type);
 
 		/* socket owned by other encapsulation module */
@@ -212,11 +214,11 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 	/* TCP sockets are per-peer, therefore they are linked to their unique
 	 * peer
 	 */
-	if (sk->sk_protocol == IPPROTO_TCP) {
+	if (tcp) {
 		INIT_WORK(&ovpn_sock->tcp_tx_work, ovpn_tcp_tx_work);
 		ovpn_sock->peer = peer;
 		ovpn_peer_hold(peer);
-	} else if (sk->sk_protocol == IPPROTO_UDP) {
+	} else if (udp) {
 		/* in UDP we only link the ovpn instance since the socket is
 		 * shared among multiple peers
 		 */
@@ -237,9 +239,9 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 
 	ret = ovpn_socket_attach(ovpn_sock, sock, peer);
 	if (ret < 0) {
-		if (sk->sk_protocol == IPPROTO_TCP)
+		if (tcp)
 			ovpn_peer_put(peer);
-		else if (sk->sk_protocol == IPPROTO_UDP)
+		else if (udp)
 			netdev_put(peer->ovpn->dev, &ovpn_sock->dev_tracker);
 
 		sock_put(sk);

[PATCH net 3/7] ovpn: reject UDP remotes incompatible with socket family

[Antonio Quartulli]

From: Ralf Lici <ralf@mandelbit.com>

ovpn accepts a userspace-provided socket and a peer remote endpoint
through netlink. For UDP peers, the remote endpoint family selects the
transmit path used later by ovpn_udp_output.

Reject UDP peer remotes that are incompatible with the socket state when
they are configured through netlink. An IPv4 UDP socket cannot be used
with an IPv6 remote endpoint, and an IPv6-only UDP socket cannot send to
an IPv4 remote endpoint. Reporting this at setup time gives userspace a
clear extack and avoids installing a peer that would immediately be
unable to transmit.

Accepting an IPv6 remote on an AF_INET socket is also unsafe. On TX,
ovpn_udp6_output() hands the socket to ip6_dst_lookup_flow(), which
dereferences inet6_sk(sk), e.g. to read srcprefs in
ip6_route_output_flags() when no local address is set. inet6_sk() is
NULL for AF_INET sockets, therefore the first packet sent to such a
peer triggers a NULL pointer dereference.

This validation only covers the socket state visible while the netlink
request is handled. The socket remains owned by userspace after it is
attached to ovpn, so userspace can later change properties such as
IPV6_V6ONLY or IPV6_ADDRFORM. The transmit path therefore still has to
re-check socket/remote compatibility before sending.

Parse the remote endpoint once in the peer new/set paths and reject UDP
remotes when the provided socket cannot send to them. Pass the parsed
endpoint into the common peer modify helper so the validation and the
stored endpoint use the same normalized sockaddr.

Fixes: 1d36a36f6d53 ("ovpn: implement peer add/get/dump/delete via netlink")
Cc: stable@vger.kernel.org
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/netlink.c | 128 ++++++++++++++++++++++++++-----------
 1 file changed, 92 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index 1af7e0e624ba..bf27a7a89f76 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -310,15 +310,17 @@ static int ovpn_nl_peer_precheck(struct ovpn_priv *ovpn,
  * @peer: the peer to modify
  * @info: generic netlink info from the user request
  * @attrs: the attributes from the user request
+ * @remote: remote address, if provided
  *
  * Return: a negative error code in case of failure, 0 on success or 1 on
  *	   success and the VPN IPs have been modified (requires rehashing in MP
  *	   mode)
  */
 static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
-			       struct nlattr **attrs)
+			       struct nlattr **attrs,
+			       const struct sockaddr_storage *remote)
 {
-	struct sockaddr_storage ss = {};
+	struct sockaddr_storage empty_remote = {};
 	void *local_ip = NULL;
 	u32 interv, timeout;
 	bool rehash = false;
@@ -326,15 +328,15 @@ static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
 
 	spin_lock_bh(&peer->lock);
 
-	if (ovpn_nl_attr_sockaddr_remote(attrs, &ss)) {
+	if (remote) {
 		/* we carry the local IP in a generic container.
 		 * ovpn_peer_reset_sockaddr() will properly interpret it
-		 * based on ss.ss_family
+		 * based on remote->ss_family
 		 */
 		local_ip = ovpn_nl_attr_local_ip(attrs);
 
 		/* set peer sockaddr */
-		ret = ovpn_peer_reset_sockaddr(peer, &ss, local_ip);
+		ret = ovpn_peer_reset_sockaddr(peer, remote, local_ip);
 		if (ret < 0) {
 			NL_SET_ERR_MSG_FMT_MOD(info->extack,
 					       "cannot set peer sockaddr: %d",
@@ -372,7 +374,7 @@ static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
 
 	netdev_dbg(peer->ovpn->dev,
 		   "modify peer id=%u tx_id=%u endpoint=%pIScp VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n",
-		   peer->id, peer->tx_id, &ss,
+		   peer->id, peer->tx_id, remote ?: &empty_remote,
 		   &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
 
 	spin_unlock_bh(&peer->lock);
@@ -383,12 +385,45 @@ static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
 	return ret;
 }
 
+/**
+ * ovpn_nl_udp_remote_compatible - check if a UDP socket can use a remote
+ * @sk: UDP socket to validate
+ * @remote: remote endpoint to validate against the socket
+ * @extack: netlink extended ACK for reporting validation errors
+ *
+ * Return: 0 if the remote endpoint is compatible with the socket or a negative
+ * error code otherwise.
+ */
+static int ovpn_nl_udp_remote_compatible(const struct sock *sk,
+					 const struct sockaddr_storage *remote,
+					 struct netlink_ext_ack *extack)
+{
+	const unsigned short sock_family = READ_ONCE(sk->sk_family);
+
+	if (sock_family == AF_INET && remote->ss_family == AF_INET6) {
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "UDP socket is IPv4 but remote is IPv6");
+		return -EAFNOSUPPORT;
+	}
+
+	if (sock_family == AF_INET6 && ipv6_only_sock(sk) &&
+	    remote->ss_family == AF_INET) {
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "UDP socket is IPv6-only but remote is IPv4");
+		return -EAFNOSUPPORT;
+	}
+
+	return 0;
+}
+
 int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct in_addr vpn_addr4 = { .s_addr = htonl(INADDR_ANY) };
+	const struct sockaddr_storage *remote = NULL;
 	struct in6_addr vpn_addr6 = IN6ADDR_ANY_INIT;
 	struct ovpn_priv *ovpn = info->user_ptr[0];
 	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
+	struct sockaddr_storage ss = {};
 	struct ovpn_socket *ovpn_sock;
 	struct socket *sock = NULL;
 	struct ovpn_peer *peer;
@@ -459,26 +494,34 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 		goto peer_release;
 	}
 
-	/* Only when using UDP as transport protocol the remote endpoint
-	 * can be configured so that ovpn knows where to send packets to.
-	 */
-	if (sk_is_udp(sock->sk) &&
-	    !attrs[OVPN_A_PEER_REMOTE_IPV4] &&
-	    !attrs[OVPN_A_PEER_REMOTE_IPV6]) {
-		NL_SET_ERR_MSG_FMT_MOD(info->extack,
-				       "missing remote IP address for UDP socket");
-		sockfd_put(sock);
-		ret = -EINVAL;
-		goto peer_release;
-	}
+	if (ovpn_nl_attr_sockaddr_remote(attrs, &ss))
+		remote = &ss;
 
-	/* In case of TCP, the socket is connected to the peer and ovpn
-	 * will just send bytes over it, without the need to specify a
-	 * destination.
-	 */
-	if (sk_is_tcp(sock->sk) &&
-	    (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
-	     attrs[OVPN_A_PEER_REMOTE_IPV6])) {
+	if (sk_is_udp(sock->sk)) {
+		/* Only when using UDP as transport protocol the remote
+		 * endpoint can be configured so that ovpn knows where to send
+		 * packets to.
+		 */
+		if (!remote) {
+			NL_SET_ERR_MSG_FMT_MOD(info->extack,
+					       "missing remote IP address for UDP socket");
+			sockfd_put(sock);
+			ret = -EINVAL;
+			goto peer_release;
+		}
+
+		/* can the socket be used with this remote? */
+		ret = ovpn_nl_udp_remote_compatible(sock->sk, remote,
+						    info->extack);
+		if (ret < 0) {
+			sockfd_put(sock);
+			goto peer_release;
+		}
+	} else if (remote) {
+		/* In case of TCP, the socket is connected to the peer and ovpn
+		 * will just send bytes over it, without the need to specify a
+		 * destination.
+		 */
 		NL_SET_ERR_MSG_FMT_MOD(info->extack,
 				       "unexpected remote IP address with TCP socket");
 		sockfd_put(sock);
@@ -504,7 +547,7 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 
 	rcu_assign_pointer(peer->sock, ovpn_sock);
 
-	ret = ovpn_nl_peer_modify(peer, info, attrs);
+	ret = ovpn_nl_peer_modify(peer, info, attrs, remote);
 	if (ret < 0)
 		goto sock_release;
 
@@ -533,8 +576,10 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 
 int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
 {
+	const struct sockaddr_storage *remote = NULL;
 	struct ovpn_priv *ovpn = info->user_ptr[0];
 	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
+	struct sockaddr_storage ss = {};
 	struct in6_addr vpn_addr6;
 	struct in_addr vpn_addr4;
 	struct ovpn_socket *sock;
@@ -568,17 +613,28 @@ int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
 		return -ENOENT;
 	}
 
-	/* when using a TCP socket the remote IP is not expected */
+	if (ovpn_nl_attr_sockaddr_remote(attrs, &ss))
+		remote = &ss;
+
 	rcu_read_lock();
 	sock = rcu_dereference(peer->sock);
-	if (sock && sock->sk->sk_protocol == IPPROTO_TCP &&
-	    (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
-	     attrs[OVPN_A_PEER_REMOTE_IPV6])) {
-		rcu_read_unlock();
-		NL_SET_ERR_MSG_FMT_MOD(info->extack,
-				       "unexpected remote IP address with TCP socket");
-		ovpn_peer_put(peer);
-		return -EINVAL;
+	if (sock && remote) {
+		if (sk_is_udp(sock->sk)) {
+			ret = ovpn_nl_udp_remote_compatible(sock->sk, remote,
+							    info->extack);
+			if (ret < 0) {
+				rcu_read_unlock();
+				ovpn_peer_put(peer);
+				return ret;
+			}
+		} else if (sk_is_tcp(sock->sk)) {
+			/* when using a TCP socket remote IP is not expected */
+			NL_SET_ERR_MSG_FMT_MOD(info->extack,
+					       "unexpected remote IP address with TCP socket");
+			rcu_read_unlock();
+			ovpn_peer_put(peer);
+			return -EINVAL;
+		}
 	}
 	rcu_read_unlock();
 
@@ -607,7 +663,7 @@ int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
 			goto unlock;
 	}
 
-	ret = ovpn_nl_peer_modify(peer, info, attrs);
+	ret = ovpn_nl_peer_modify(peer, info, attrs, remote);
 	if (ret < 0)
 		goto unlock;
 

Re: [PATCH net 3/7] ovpn: reject UDP remotes incompatible with socket family

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In this commit, ovpn_nl_udp_remote_compatible()
  (drivers/net/ovpn/netlink.c) checks sk->sk_family and…
- [Low] The concern says the new rejection in
  ovpn_nl_udp_remote_compatible() (AF_INET6 socket + ipv6_only_sock() +…
- [Low] The concern says the NULL inet6_sk(sk) dereference the commit
  message describes is still reachable through the RX float path.

--

ovpn: reject UDP remotes incompatible with socket family

This commit adds ovpn_nl_udp_remote_compatible(). It rejects UDP remotes
that the attached socket cannot use when a peer is created or modified
over netlink: an IPv6 remote on an AF_INET socket, or an IPv4 remote on an
IPv6-only socket. The remote endpoint is now parsed once in the new/set
paths and passed to ovpn_nl_peer_modify(), and the commit message notes
that the transmit path still has to re-check the socket later.

> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
> index 1af7e0e624ba6..bf27a7a89f76b 100644
> --- a/drivers/net/ovpn/netlink.c
> +++ b/drivers/net/ovpn/netlink.c

[ ... ]

> @@ -383,12 +385,45 @@ static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
>  	return ret;
>  }
>  
> +/**
> + * ovpn_nl_udp_remote_compatible - check if a UDP socket can use a remote

[ ... ]

> +static int ovpn_nl_udp_remote_compatible(const struct sock *sk,
> +					 const struct sockaddr_storage *remote,
> +					 struct netlink_ext_ack *extack)
> +{
> +	const unsigned short sock_family = READ_ONCE(sk->sk_family);

[Severity: Low]
This check reads sk->sk_family and ipv6_only_sock(sk) once, without
lock_sock(), while the PEER_NEW/PEER_SET request is handled. In
ovpn_nl_peer_set_doit() it runs under rcu_read_lock(), and that lock is
dropped before ovpn_nl_peer_modify() installs the new bind.

The commit message says:

  The transmit path therefore still has to re-check socket/remote
  compatibility before sending.

At this commit, ovpn_udp_output() still picks ovpn_udp4_output() or
ovpn_udp6_output() based only on bind->remote.in4.sin_family. A later
IPV6_V6ONLY or IPV6_ADDRFORM change is therefore not caught on TX.

Is this patch meant to be complete on its own for stable? The next patch
in the series, "ovpn: recheck UDP socket family before transmit", adds the
sock_family check in ovpn_udp_output() and closes this gap. It looks like
the two patches need to be backported together.

> +
> +	if (sock_family == AF_INET && remote->ss_family == AF_INET6) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "UDP socket is IPv4 but remote is IPv6");
> +		return -EAFNOSUPPORT;
> +	}

[Severity: Low]
This rejects an IPv6 remote on an AF_INET socket when the remote comes from
netlink. Can the same mismatched bind still be installed through the RX
float path, which does not go through this helper?

drivers/net/ovpn/io.c:ovpn_recv() {
    ...
	if (sock && sock->sk->sk_protocol == IPPROTO_UDP)
		/* check if this peer changed local or remote endpoint */
		ovpn_peer_endpoints_update(peer, skb);
    ...
}

ovpn_peer_endpoints_update() updates the bind from the received packet and
does not look at the socket family. If the bind becomes AF_INET6 on a peer
whose socket is AF_INET, the next transmit would take this path:

ovpn_udp_output()
  ovpn_udp6_output()
    ip6_dst_lookup_flow(sock_net(sk), sk, &fl, NULL)

That path reaches the inet6_sk(sk) NULL dereference described in the
commit message.

At the end of the series, "ovpn: recheck UDP socket family before transmit"
covers this case. Before calling ovpn_udp6_output(), ovpn_udp_output() now
rejects AF_INET6 binds when sock_family != AF_INET6. With that patch, the
only remaining effect is that the floating peer is deleted with a transport
error.

Without that patch, does this commit alone prevent the crash it describes?

> +
> +	if (sock_family == AF_INET6 && ipv6_only_sock(sk) &&
> +	    remote->ss_family == AF_INET) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "UDP socket is IPv6-only but remote is IPv4");
> +		return -EAFNOSUPPORT;
> +	}

[Severity: Low]
This isn't a bug, but I have a question about this part of the commit
message:

  An IPv4 UDP socket cannot be used with an IPv6 remote endpoint, and an
  IPv6-only UDP socket cannot send to an IPv4 remote endpoint.

At this commit, ovpn_udp4_output() gets the route with
ip_route_output_flow(sock_net(sk), &fl, sk) and does not check
ipv6_only_sock(sk). So ovpn's own TX path would still send in this
configuration.

What actually makes the configuration unusable is the receive side, since
compute_score() in net/ipv4/udp.c skips IPv6-only sockets. The regular
udpv6_sendmsg() behaviour for v4-mapped destinations points the same way.

The next patch in the series adds the matching ipv6_only_sock() check in
ovpn_udp_output(), so the netlink and TX rules agree by the end of the
series. Since this patch rejects a configuration that used to be accepted,
would it be worth making the reasoning clearer here?

> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022121606.732029-2-antonio%40openvpn.net

[PATCH net 4/7] ovpn: recheck UDP socket family before transmit

[Antonio Quartulli]

From: Ralf Lici <ralf@mandelbit.com>

ovpn validates UDP peer remotes against the socket family when the
remote endpoint is configured through netlink. The socket itself,
however, remains owned by userspace and some socket options can still
change the family seen by the transmit path.

For example, IPV6_ADDRFORM can turn an AF_INET6 socket into AF_INET
after ovpn accepted an IPv6 remote. Conversely, IPV6_V6ONLY can make a
dual-stack AF_INET6 socket unable to send to an IPv4 remote.

The IPv6 TX path relies on the socket carrying IPv6 state, as
ip6_dst_lookup_flow() dereferences inet6_sk(sk), which is NULL for
native AF_INET sockets. Checking the family at transmit time
guarantees that ovpn_udp6_output() is only reached with a socket that
has valid IPv6 state, regardless of how the peer remote was installed.

Recheck the socket family in ovpn_udp_output before selecting the UDP
transmit path. When the peer remote family no longer matches the socket
state, emit a ratelimited warning, drop the packet with -EAFNOSUPPORT
and delete the peer with TRANSPORT_ERROR.

Peer deletion is deferred through a common transport-error work item,
because the UDP transmit path can run from non-sleepable contexts while
ovpn_peer_del may release sockets and sleep. Use the same helper for TCP
transport errors instead of keeping a TCP-only deferred delete work
item.

Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
Cc: stable@vger.kernel.org
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
[antonio: use queue_work(ovpn_wq) instead of schedule_work() to
 follow the module-owned workqueue conversion]
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/peer.c | 19 +++++++++++++++++++
 drivers/net/ovpn/peer.h |  5 +++--
 drivers/net/ovpn/tcp.c  | 23 +++--------------------
 drivers/net/ovpn/udp.c  | 24 +++++++++++++++++++++---
 4 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index 2067825bb5b6..5e22015351eb 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -83,6 +83,24 @@ static void ovpn_peer_keepalive_send(struct work_struct *work)
 	local_bh_enable();
 }
 
+static void ovpn_peer_transport_error_work(struct work_struct *work)
+{
+	struct ovpn_peer *peer = container_of(work, struct ovpn_peer,
+					      transport_error_work);
+
+	ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR);
+	ovpn_peer_put(peer);
+}
+
+void ovpn_peer_del_transport_error(struct ovpn_peer *peer)
+{
+	if (WARN_ON(!ovpn_peer_hold(peer)))
+		return;
+
+	if (!queue_work(ovpn_wq, &peer->transport_error_work))
+		ovpn_peer_put(peer);
+}
+
 /**
  * ovpn_peer_new - allocate and initialize a new peer object
  * @ovpn: the openvpn instance inside which the peer should be created
@@ -117,6 +135,7 @@ struct ovpn_peer *ovpn_peer_new(struct ovpn_priv *ovpn, u32 id)
 	kref_init(&peer->refcount);
 	ovpn_peer_stats_init(&peer->vpn_stats);
 	ovpn_peer_stats_init(&peer->link_stats);
+	INIT_WORK(&peer->transport_error_work, ovpn_peer_transport_error_work);
 	INIT_WORK(&peer->keepalive_work, ovpn_peer_keepalive_send);
 
 	ret = dst_cache_init(&peer->dst_cache, GFP_KERNEL);
diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index 1879bfb76992..4b20a421cd40 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h
@@ -72,6 +72,7 @@ struct ovpn_route_key {
  * @refcount: reference counter
  * @rcu: used to free peer in an RCU safe way
  * @release_entry: entry for the socket release list
+ * @transport_error_work: work used to delete peer on transport error
  * @keepalive_work: used to schedule keepalive sending
  */
 struct ovpn_peer {
@@ -107,8 +108,6 @@ struct ovpn_peer {
 			struct proto *prot;
 			const struct proto_ops *ops;
 		} sk_cb;
-
-		struct work_struct defer_del_work;
 	} tcp;
 	struct ovpn_crypto_state crypto;
 	struct dst_cache dst_cache;
@@ -128,6 +127,7 @@ struct ovpn_peer {
 	struct kref refcount;
 	struct rcu_head rcu;
 	struct llist_node release_entry;
+	struct work_struct transport_error_work;
 	struct work_struct keepalive_work;
 };
 
@@ -156,6 +156,7 @@ static inline void ovpn_peer_put(struct ovpn_peer *peer)
 struct ovpn_peer *ovpn_peer_new(struct ovpn_priv *ovpn, u32 id);
 int ovpn_peer_add(struct ovpn_priv *ovpn, struct ovpn_peer *peer);
 int ovpn_peer_del(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason);
+void ovpn_peer_del_transport_error(struct ovpn_peer *peer);
 void ovpn_peers_free(struct ovpn_priv *ovpn, struct sock *sock,
 		     enum ovpn_del_peer_reason reason);
 
diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 8fe8a8e750a4..5340860c0b82 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -148,11 +148,7 @@ static void ovpn_tcp_rcv(struct strparser *strp, struct sk_buff *skb)
 	ovpn_recv(peer, skb);
 	return;
 err:
-	/* take reference for deferred peer deletion. should never fail */
-	if (WARN_ON(!ovpn_peer_hold(peer)))
-		goto err_nopeer;
-	if (!queue_work(ovpn_wq, &peer->tcp.defer_del_work))
-		ovpn_peer_put(peer);
+	ovpn_peer_del_transport_error(peer);
 	ovpn_dev_dstats_rx_dropped(peer->ovpn->dev);
 err_nopeer:
 	kfree_skb(skb);
@@ -240,7 +236,7 @@ void ovpn_tcp_socket_wait_finish(struct ovpn_socket *sock)
 {
 	struct ovpn_peer *peer = sock->peer;
 
-	/* NOTE: we don't wait for peer->tcp.defer_del_work to finish:
+	/* NOTE: we don't wait for peer->transport_error_work to finish:
 	 * either the worker is not running or this function
 	 * was invoked by that worker.
 	 */
@@ -283,9 +279,7 @@ static void ovpn_tcp_send_sock(struct ovpn_peer *peer, struct sock *sk)
 			/* in case of TCP error we can't recover the VPN
 			 * stream therefore we abort the connection
 			 */
-			ovpn_peer_hold(peer);
-			if (!queue_work(ovpn_wq, &peer->tcp.defer_del_work))
-				ovpn_peer_put(peer);
+			ovpn_peer_del_transport_error(peer);
 
 			/* we bail out immediately and keep tx_in_progress set
 			 * to true. This way we prevent more TX attempts
@@ -497,15 +491,6 @@ static void ovpn_tcp_build_protos(struct proto *new_prot,
 				  const struct proto *orig_prot,
 				  const struct proto_ops *orig_ops);
 
-static void ovpn_tcp_peer_del_work(struct work_struct *work)
-{
-	struct ovpn_peer *peer = container_of(work, struct ovpn_peer,
-					      tcp.defer_del_work);
-
-	ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR);
-	ovpn_peer_put(peer);
-}
-
 /* Set TCP encapsulation callbacks */
 int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 			   struct ovpn_peer *peer)
@@ -538,8 +523,6 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 		goto err;
 	}
 
-	INIT_WORK(&peer->tcp.defer_del_work, ovpn_tcp_peer_del_work);
-
 	__sk_dst_reset(ovpn_sock->sk);
 	skb_queue_head_init(&peer->tcp.user_queue);
 	skb_queue_head_init(&peer->tcp.out_queue);
diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index 30c7eb261e7b..7672094a6766 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -416,6 +416,7 @@ static int ovpn_udp_output(struct ovpn_peer *peer, struct dst_cache *cache,
 			   struct sock *sk, struct sk_buff *skb,
 			   struct ovpn_route_key *key)
 {
+	unsigned short sock_family;
 	struct ovpn_bind *bind;
 	int ret;
 
@@ -432,18 +433,35 @@ static int ovpn_udp_output(struct ovpn_peer *peer, struct dst_cache *cache,
 		goto out;
 	}
 
+	sock_family = READ_ONCE(sk->sk_family);
+	ret = -EAFNOSUPPORT;
 	switch (bind->remote.in4.sin_family) {
 	case AF_INET:
+		/* userspace might have set IPV6_V6ONLY */
+		if (unlikely(sock_family == AF_INET6 && ipv6_only_sock(sk))) {
+			net_warn_ratelimited("%s: peer %u: IPv4 remote, IPv6-only socket\n",
+					     netdev_name(peer->ovpn->dev),
+					     peer->id);
+			ovpn_peer_del_transport_error(peer);
+			break;
+		}
+
 		ret = ovpn_udp4_output(peer, bind, cache, sk, skb, key);
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
+		/* userspace might have set IPV6_ADDRFORM */
+		if (unlikely(sock_family != AF_INET6)) {
+			net_warn_ratelimited("%s: peer %u: IPv6 remote, IPv4 socket\n",
+					     netdev_name(peer->ovpn->dev),
+					     peer->id);
+			ovpn_peer_del_transport_error(peer);
+			break;
+		}
+
 		ret = ovpn_udp6_output(peer, bind, cache, sk, skb, key);
 		break;
 #endif
-	default:
-		ret = -EAFNOSUPPORT;
-		break;
 	}
 
 out:

[PATCH net 5/7] ovpn: fix UDP TX byte accounting

[Antonio Quartulli]

From: Ralf Lici <ralf@mandelbit.com>

ovpn hands encrypted data packets to udp_tunnel_xmit_skb for UDP
transport transmission. At that point skb->data points at the encrypted
OpenVPN packet, but the skb may still carry inner-header metadata
inherited from the plaintext packet.

The UDP tunnel output path uses skb_inner_network_offset to compute the
packet length passed to iptunnel_xmit_stats. If the inner network header
still points at the pre-encryption payload, the ovpn netdev TX byte
counter does not account the OpenVPN data-channel header and
authentication tag.

Reset the inner network header before handing the skb to the UDP tunnel
helpers, so the encrypted OpenVPN packet is accounted as the UDP tunnel
payload.

Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
Cc: stable@vger.kernel.org
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/udp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index 7672094a6766..05e10a2ed179 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -495,6 +495,9 @@ int ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 		return -EADDRNOTAVAIL;
 	}
 
+	/* the encrypted openvpn packet is the UDP tunnel payload */
+	skb_reset_inner_network_header(skb);
+
 	skb->dev = peer->ovpn->dev;
 	skb->mark = key.mark;
 	/* no checksum performed at this layer */

Re: [PATCH net 5/7] ovpn: fix UDP TX byte accounting

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, the ovpn netdev's rx_bytes and tx_bytes
  measure different layers.

--

ovpn: fix UDP TX byte accounting

This commit resets the inner network header in ovpn_udp_send_skb() before
the skb is handed to the UDP tunnel helpers. The goal is for the ovpn
netdev TX byte counter to include the OpenVPN data-channel header and the
authentication tag.

> diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
> index 7672094a67668..05e10a2ed1791 100644
> --- a/drivers/net/ovpn/udp.c
> +++ b/drivers/net/ovpn/udp.c
> @@ -495,6 +495,9 @@ int ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
>  		return -EADDRNOTAVAIL;
>  	}
>  
> +	/* the encrypted openvpn packet is the UDP tunnel payload */
> +	skb_reset_inner_network_header(skb);
> +

[Severity: Medium]
With this change, do rx_bytes and tx_bytes on the ovpn netdev now measure
different layers?

UDP and TCP share the same RX path, and it counts the decrypted plaintext
length:

drivers/net/ovpn/io.c:ovpn_netdev_write() {
    ...
	pkt_len = skb->len;
    ...
	if (likely(ret == NET_RX_SUCCESS)) {
		/* update RX stats with the size of decrypted packet */
		ovpn_peer_stats_increment_rx(&peer->vpn_stats, pkt_len);
		dev_dstats_rx_add(peer->ovpn->dev, pkt_len);
	}
    ...
}

UDP TX goes through iptunnel_xmit() / ip6tunnel_xmit(), which compute:

	int pkt_len = skb->len - skb_inner_network_offset(skb);

The inner network header now sits at the start of the encrypted packet.
So TX counts the opcode/peer-id (4 bytes), the packet-id (4 bytes), the
auth tag (16 bytes) and the ciphertext. That is about 24 bytes per packet
that TX counts and RX does not.

In effect, netdev TX now follows the per-peer link_stats semantics, while
netdev RX follows vpn_stats.

TCP TX uses yet another convention. ovpn_tcp_send_sock() calls
dev_dstats_tx_add(peer->ovpn->dev, skb->len) after ovpn_tcp_send_skb() has
pushed the 2-byte length prefix:

	*(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);

Other users of the same UDP tunnel stats helper do the opposite. WireGuard
sets the inner network header before it pushes its own header, so that
header is not counted:

drivers/net/wireguard/send.c:encrypt_packet() {
    ...
	skb_set_inner_network_header(skb, 0);
	header = (struct message_data *)skb_push(skb, sizeof(*header));
    ...
}

Take the case the commit message calls an undercount, where the inner
header still points at the plaintext. Before this patch, UDP TX counted the
ciphertext length there. Since the tag sits in front, that equals the
plaintext length, so it matched RX.

Is TX meant to include protocol overhead when RX does not? If so, could
the commit message explain why? If not, should RX and TCP TX be changed
to use the same convention?

The mismatch is still there at the end of the series. "ovpn: fix TCP TX
accounting for locally dropped packets" leaves both the TCP TX
dev_dstats_tx_add(skb->len) call and the RX accounting unchanged.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022121606.732029-2-antonio%40openvpn.net

Re: [PATCH net 5/7] ovpn: fix UDP TX byte accounting

[Antonio Quartulli]

I'll have a deeper look.
Some concerns, especially about the stats, seem legit.

pw-bot: cr

On 23/10/2026 14:17, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] After this patch, the ovpn netdev's rx_bytes and tx_bytes
>    measure different layers.
> 
> --
> 
> ovpn: fix UDP TX byte accounting
> 
> This commit resets the inner network header in ovpn_udp_send_skb() before
> the skb is handed to the UDP tunnel helpers. The goal is for the ovpn
> netdev TX byte counter to include the OpenVPN data-channel header and the
> authentication tag.
> 
>> diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
>> index 7672094a67668..05e10a2ed1791 100644
>> --- a/drivers/net/ovpn/udp.c
>> +++ b/drivers/net/ovpn/udp.c
>> @@ -495,6 +495,9 @@ int ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
>>   		return -EADDRNOTAVAIL;
>>   	}
>>   
>> +	/* the encrypted openvpn packet is the UDP tunnel payload */
>> +	skb_reset_inner_network_header(skb);
>> +
> 
> [Severity: Medium]
> With this change, do rx_bytes and tx_bytes on the ovpn netdev now measure
> different layers?
> 
> UDP and TCP share the same RX path, and it counts the decrypted plaintext
> length:
> 
> drivers/net/ovpn/io.c:ovpn_netdev_write() {
>      ...
> 	pkt_len = skb->len;
>      ...
> 	if (likely(ret == NET_RX_SUCCESS)) {
> 		/* update RX stats with the size of decrypted packet */
> 		ovpn_peer_stats_increment_rx(&peer->vpn_stats, pkt_len);
> 		dev_dstats_rx_add(peer->ovpn->dev, pkt_len);
> 	}
>      ...
> }
> 
> UDP TX goes through iptunnel_xmit() / ip6tunnel_xmit(), which compute:
> 
> 	int pkt_len = skb->len - skb_inner_network_offset(skb);
> 
> The inner network header now sits at the start of the encrypted packet.
> So TX counts the opcode/peer-id (4 bytes), the packet-id (4 bytes), the
> auth tag (16 bytes) and the ciphertext. That is about 24 bytes per packet
> that TX counts and RX does not.
> 
> In effect, netdev TX now follows the per-peer link_stats semantics, while
> netdev RX follows vpn_stats.
> 
> TCP TX uses yet another convention. ovpn_tcp_send_sock() calls
> dev_dstats_tx_add(peer->ovpn->dev, skb->len) after ovpn_tcp_send_skb() has
> pushed the 2-byte length prefix:
> 
> 	*(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);
> 
> Other users of the same UDP tunnel stats helper do the opposite. WireGuard
> sets the inner network header before it pushes its own header, so that
> header is not counted:
> 
> drivers/net/wireguard/send.c:encrypt_packet() {
>      ...
> 	skb_set_inner_network_header(skb, 0);
> 	header = (struct message_data *)skb_push(skb, sizeof(*header));
>      ...
> }
> 
> Take the case the commit message calls an undercount, where the inner
> header still points at the plaintext. Before this patch, UDP TX counted the
> ciphertext length there. Since the tag sits in front, that equals the
> plaintext length, so it matched RX.
> 
> Is TX meant to include protocol overhead when RX does not? If so, could
> the commit message explain why? If not, should RX and TCP TX be changed
> to use the same convention?
> 
> The mismatch is still there at the end of the series. "ovpn: fix TCP TX
> accounting for locally dropped packets" leaves both the TCP TX
> dev_dstats_tx_add(skb->len) call and the RX accounting unchanged.
> 

-- 
Antonio Quartulli
OpenVPN Inc.

[PATCH net 6/7] ovpn: fix TCP TX accounting for locally dropped packets

[Antonio Quartulli]

From: Ralf Lici <ralf@mandelbit.com>

ovpn_encrypt_post updates peer TX stats and last_sent after handing an
encrypted skb to the configured transport. This is wrong for TCP when
the transport path rejects the skb locally: ovpn_tcp_send_skb can fail
when the per-peer output queue is full, or when another output skb is
still busy.

Make the TCP send helpers return an error when they cannot consume the
skb. For the encrypted data path, keep ownership in ovpn_encrypt_post on
error so it can account and free the drop without updating peer TX stats
or last_sent. Handle the same helper errors locally in TCP release and
sendmsg paths, which do not go through ovpn_encrypt_post.

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Cc: stable@vger.kernel.org
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c  |  4 ++-
 drivers/net/ovpn/tcp.c | 55 ++++++++++++++++++++++++++++++------------
 drivers/net/ovpn/tcp.h | 10 ++------
 3 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index ce1993b243e6..eb9ba709523e 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -287,7 +287,9 @@ void ovpn_encrypt_post(void *data, int ret)
 			goto err_unlock;
 		break;
 	case IPPROTO_TCP:
-		ovpn_tcp_send_skb(peer, sock->sk, skb);
+		ret = ovpn_tcp_send_skb(peer, sock->sk, skb);
+		if (unlikely(ret < 0))
+			goto err_unlock;
 		break;
 	default:
 		/* no transport configured yet */
diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 5340860c0b82..a3165580e97b 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -319,28 +319,39 @@ void ovpn_tcp_tx_work(struct work_struct *work)
 	release_sock(sock->sk);
 }
 
-static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
-				   struct sk_buff *skb)
+static int ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
+				  struct sk_buff *skb)
 {
 	if (peer->tcp.out_msg.skb)
 		ovpn_tcp_send_sock(peer, sk);
 
-	if (peer->tcp.out_msg.skb) {
-		ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
-		kfree_skb(skb);
-		return;
-	}
+	if (peer->tcp.out_msg.skb)
+		return -EBUSY;
 
 	peer->tcp.out_msg.skb = skb;
 	peer->tcp.out_msg.len = skb->len;
 	peer->tcp.out_msg.offset = 0;
 	ovpn_tcp_send_sock(peer, sk);
+	return 0;
 }
 
-void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
-		       struct sk_buff *skb)
+/**
+ * ovpn_tcp_send_skb - Prepare skb and enqueue it for sending to peer
+ * @peer: destination peer
+ * @sk: transport socket
+ * @skb: packet to send
+ *
+ * Prepends the skb payload length, as required by the OpenVPN protocol in
+ * order to extract packets from the TCP stream on the receiver side.
+ *
+ * Return: 0 on success or a negative error code otherwise. On failure, the
+ * caller retains ownership of @skb.
+ */
+int ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
+		      struct sk_buff *skb)
 {
 	u16 len = skb->len;
+	int ret = 0;
 
 	*(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);
 
@@ -348,16 +359,16 @@ void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 	if (sock_owned_by_user(sk)) {
 		if (skb_queue_len(&peer->tcp.out_queue) >=
 		    READ_ONCE(net_hotdata.max_backlog)) {
-			ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
-			kfree_skb(skb);
+			ret = -ENOBUFS;
 			goto unlock;
 		}
 		__skb_queue_tail(&peer->tcp.out_queue, skb);
 	} else {
-		ovpn_tcp_send_sock_skb(peer, sk, skb);
+		ret = ovpn_tcp_send_sock_skb(peer, sk, skb);
 	}
 unlock:
 	spin_unlock(&sk->sk_lock.slock);
+	return ret;
 }
 
 static void ovpn_tcp_release(struct sock *sk)
@@ -366,6 +377,7 @@ static void ovpn_tcp_release(struct sock *sk)
 	struct ovpn_socket *sock;
 	struct ovpn_peer *peer;
 	struct sk_buff *skb;
+	int ret;
 
 	rcu_read_lock();
 	sock = rcu_dereference_sk_user_data(sk);
@@ -388,8 +400,13 @@ static void ovpn_tcp_release(struct sock *sk)
 	__skb_queue_head_init(&queue);
 	skb_queue_splice_init(&peer->tcp.out_queue, &queue);
 
-	while ((skb = __skb_dequeue(&queue)))
-		ovpn_tcp_send_sock_skb(peer, sk, skb);
+	while ((skb = __skb_dequeue(&queue))) {
+		ret = ovpn_tcp_send_sock_skb(peer, sk, skb);
+		if (unlikely(ret < 0)) {
+			ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
+			kfree_skb(skb);
+		}
+	}
 
 	peer->tcp.sk_cb.prot->release_cb(sk);
 	ovpn_peer_put(peer);
@@ -447,8 +464,14 @@ static int ovpn_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	}
 
 	ovpn_skb_cb(skb)->nosignal = msg->msg_flags & MSG_NOSIGNAL;
-	ovpn_tcp_send_sock_skb(peer, sk, skb);
-	ret = size;
+	ret = ovpn_tcp_send_sock_skb(peer, sk, skb);
+	if (unlikely(ret < 0)) {
+		ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
+		kfree_skb(skb);
+	} else {
+		ret = size;
+	}
+
 peer_free:
 	release_sock(sk);
 	ovpn_peer_put(peer);
diff --git a/drivers/net/ovpn/tcp.h b/drivers/net/ovpn/tcp.h
index a3aa3570ae5e..957f7f7db6c8 100644
--- a/drivers/net/ovpn/tcp.h
+++ b/drivers/net/ovpn/tcp.h
@@ -24,14 +24,8 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 void ovpn_tcp_socket_detach(struct ovpn_socket *ovpn_sock);
 void ovpn_tcp_socket_wait_finish(struct ovpn_socket *sock);
 
-/* Prepare skb and enqueue it for sending to peer.
- *
- * Preparation consist in prepending the skb payload with its size.
- * Required by the OpenVPN protocol in order to extract packets from
- * the TCP stream on the receiver side.
- */
-void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
-		       struct sk_buff *skb);
+int ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
+		      struct sk_buff *skb);
 void ovpn_tcp_tx_work(struct work_struct *work);
 
 #endif /* _NET_OVPN_TCP_H_ */

Re: [PATCH net 6/7] ovpn: fix TCP TX accounting for locally dropped packets

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix leaves a TCP local-drop path with the same wrong
  accounting.

--

ovpn: fix TCP TX accounting for locally dropped packets

With this patch, ovpn_tcp_send_skb() and ovpn_tcp_send_sock_skb() return
an error when TCP cannot take the skb, instead of freeing it. On error,
ovpn_encrypt_post() keeps the skb, counts the drop and frees it, without
updating peer TX stats or last_sent. The TCP release and sendmsg paths
now count and free the skb themselves when these helpers fail.

> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> index ce1993b243e6f..eb9ba709523ee 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -287,7 +287,9 @@ void ovpn_encrypt_post(void *data, int ret)
>  			goto err_unlock;
>  		break;
>  	case IPPROTO_TCP:
> -		ovpn_tcp_send_skb(peer, sock->sk, skb);
> +		ret = ovpn_tcp_send_skb(peer, sock->sk, skb);
> +		if (unlikely(ret < 0))
> +			goto err_unlock;
>  		break;
>  	default:
>  		/* no transport configured yet */
> diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
> index 5340860c0b822..a3165580e97b5 100644
> --- a/drivers/net/ovpn/tcp.c
> +++ b/drivers/net/ovpn/tcp.c
> @@ -319,28 +319,39 @@ void ovpn_tcp_tx_work(struct work_struct *work)
>  	release_sock(sock->sk);
>  }
>  
> -static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
> -				   struct sk_buff *skb)
> +static int ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
> +				  struct sk_buff *skb)
>  {
>  	if (peer->tcp.out_msg.skb)
>  		ovpn_tcp_send_sock(peer, sk);
>  
> -	if (peer->tcp.out_msg.skb) {
> -		ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
> -		kfree_skb(skb);
> -		return;
> -	}
> +	if (peer->tcp.out_msg.skb)
> +		return -EBUSY;

[ ... ]

> @@ -348,16 +359,16 @@ void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
>  	if (sock_owned_by_user(sk)) {
>  		if (skb_queue_len(&peer->tcp.out_queue) >=
>  		    READ_ONCE(net_hotdata.max_backlog)) {
> -			ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
> -			kfree_skb(skb);
> +			ret = -ENOBUFS;
>  			goto unlock;
>  		}
>  		__skb_queue_tail(&peer->tcp.out_queue, skb);
>  	} else {
> -		ovpn_tcp_send_sock_skb(peer, sk, skb);
> +		ret = ovpn_tcp_send_sock_skb(peer, sk, skb);
>  	}

[ ... ]

> @@ -388,8 +400,13 @@ static void ovpn_tcp_release(struct sock *sk)
>  	__skb_queue_head_init(&queue);
>  	skb_queue_splice_init(&peer->tcp.out_queue, &queue);
>  
> -	while ((skb = __skb_dequeue(&queue)))
> -		ovpn_tcp_send_sock_skb(peer, sk, skb);
> +	while ((skb = __skb_dequeue(&queue))) {
> +		ret = ovpn_tcp_send_sock_skb(peer, sk, skb);
> +		if (unlikely(ret < 0)) {
> +			ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
> +			kfree_skb(skb);
> +		}
> +	}

[Severity: High]
Can skbs that ovpn_encrypt_post() already counted as sent still be
dropped here?

When sock_owned_by_user(sk) is true, ovpn_tcp_send_skb() only puts the skb
on peer->tcp.out_queue and returns 0. ovpn_encrypt_post() treats that 0 as
a successful send:

ovpn_encrypt_post() {
    ...
	ovpn_peer_stats_increment_tx(&peer->link_stats, orig_len);
	/* keep track of last sent packet for keepalive */
	WRITE_ONCE(peer->last_sent, ktime_get_boottime_seconds());
    ...
}

Later, release_sock()->ovpn_tcp_release() drains out_queue through
ovpn_tcp_send_sock_skb(). Suppose peer->tcp.out_msg.skb is still pending
because skb_send_sock_locked_with_flags() returned -EAGAIN under TCP
backpressure. Then every drained skb gets -EBUSY and lands in this new
branch.

The packet is then counted in the peer link TX counters
(OVPN_A_PEER_LINK_TX_BYTES/PACKETS) and also in the device tx_dropped
counter. peer->last_sent was also refreshed for a packet that never left.
That pushes back the keepalive deadline in
ovpn_peer_keepalive_work_single().

Isn't this the "another output skb is still busy" case the commit message
says it fixes? Several contexts can hold lock_sock() here. One is
ovpn_tcp_tx_work(), which gets queued exactly when out_msg is stuck.
Others are ovpn_tcp_sendmsg() and a userspace setsockopt(). A remote peer
that stops reading (zero window) can keep out_msg stuck.

The commit message also says:

    Handle the same helper errors locally in TCP release and
    sendmsg paths, which do not go through ovpn_encrypt_post.

That doesn't look right for the release path. Only ovpn_tcp_send_skb()
fills out_queue, and only ovpn_encrypt_post() calls ovpn_tcp_send_skb().
So every skb dropped here was already counted as sent. The same goes for
queued skbs freed by skb_queue_purge() in ovpn_tcp_socket_detach().

The selftests change later in the series does not touch this code, so
this is still present at the end of the series.

Could the peer link TX accounting and the last_sent update move to the
point where TCP actually accepts the skb into out_msg or sends it?
ovpn_tcp_send_sock() already does this for dev_dstats_tx_add().
Alternatively, the accounting could be undone when a queued skb is
dropped.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022121606.732029-2-antonio%40openvpn.net

[PATCH net 7/7] selftests: ovpn: avoid calling freeaddrinfo() on NULL

[Antonio Quartulli]

ovpn_parse_remote() always calls freeaddrinfo() on its exit path, but
result is NULL when no vpnip was passed (with or without a remote host).

POSIX does not allow passing NULL to freeaddrinfo(). glibc tolerates
it, while musl dereferences the pointer and ovpn-cli segfaults.

Only free result when getaddrinfo() actually returned a list.

Fixes: 959bc330a439 ("testing/selftests: add test tool and scripts for ovpn module")
Cc: stable@vger.kernel.org
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 tools/testing/selftests/net/ovpn/ovpn-cli.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c
index 3b612a8a18fe..ce9436f92b43 100644
--- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
+++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
@@ -1849,7 +1849,8 @@ static int ovpn_parse_remote(struct ovpn_ctx *ovpn, const char *host,
 
 	ret = 0;
 out:
-	freeaddrinfo(result);
+	if (result)
+		freeaddrinfo(result);
 	return ret;
 }
 

Re: [PATCH net 7/7] selftests: ovpn: avoid calling freeaddrinfo() on NULL

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The Fixes: tag in commit "selftests: ovpn: avoid calling
  freeaddrinfo() on NULL" names the wrong commit.

--

selftests: ovpn: avoid calling freeaddrinfo() on NULL

This patch changes the exit path of ovpn_parse_remote() in
tools/testing/selftests/net/ovpn/ovpn-cli.c so that freeaddrinfo() is
called only when result is non-NULL. On musl, passing NULL to
freeaddrinfo() makes ovpn-cli segfault.

> ovpn_parse_remote() always calls freeaddrinfo() on its exit path, but
> result is NULL when no vpnip was passed (with or without a remote host).

[ ... ]

> Fixes: 959bc330a439 ("testing/selftests: add test tool and scripts for ovpn module")
> Cc: stable@vger.kernel.org

[Severity: Low]
Does this Fixes: tag point at the right commit?

In 959bc330a439, result had no initializer, and the host branch in
ovpn_parse_remote() did not free or reset it:

    struct addrinfo *result;
    ...
        memcpy(&ovpn->remote, result->ai_addr, result->ai_addrlen);
    }

So with a host and no vpnip, the out: label freed the valid list that
getaddrinfo() returned. The CMD_CONNECT path in main() is one such caller:

    ret = ovpn_parse_new_peer(ovpn, argv[3], argv[4], argv[5],
                              argv[6], NULL);

With no host and no vpnip, out: received an uninitialized pointer, not
NULL.

The NULL case looks like it came from commit 0bd9cfebc1c9
("selftests/net: ovpn: fix getaddrinfo memory leak in
ovpn_parse_remote()"). That commit added both of these:

    struct addrinfo *result = NULL;
    ...
        freeaddrinfo(result);
        result = NULL;

The commit message of 0bd9cfebc1c9 also describes the no-host, no-vpnip
case in 959bc330a439 as an uninitialized pointer.

So the statement "result is NULL when no vpnip was passed (with or
without a remote host)" only holds once 0bd9cfebc1c9 is applied. Should
the tag be this instead?

    Fixes: 0bd9cfebc1c9 ("selftests/net: ovpn: fix getaddrinfo memory leak in ovpn_parse_remote()")

The context of the modified hunk has not changed since 959bc330a439. With
Cc: stable, the patch would also apply cleanly to stable trees that lack
0bd9cfebc1c9.

In those trees, wouldn't the new if (result) check test an uninitialized
pointer on the no-host, no-vpnip path, so the patch fixes nothing there?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022121606.732029-2-antonio%40openvpn.net