[PATCH net-next 2/4] ovpn: ensure sk is still valid during cleanup

[PATCH net-next 2/4] ovpn: ensure sk is still valid during cleanup

[Antonio Quartulli]

Removing a peer while userspace attempts to close its transport
socket triggers a race condition resulting in the following
crash:

Oops: general protection fault, probably for non-canonical address 0xdffffc0000000077: 0000 [#1] SMP KASAN
KASAN: null-ptr-deref in range [0x00000000000003b8-0x00000000000003bf]
CPU: 12 UID: 0 PID: 162 Comm: kworker/12:1 Tainted: G           O        6.15.0-rc2-00635-g521139ac3840 #272 PREEMPT(full)
Tainted: [O]=OOT_MODULE
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-20240910_120124-localhost 04/01/2014
Workqueue: events ovpn_peer_keepalive_work [ovpn]
RIP: 0010:ovpn_socket_release+0x23c/0x500 [ovpn]
Code: ea 03 80 3c 02 00 0f 85 71 02 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 64 24 18 49 8d bc 24 be 03 00 00 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 01 38 d0 7c 08 84 d2 0f 85 30
RSP: 0018:ffffc90000c9fb18 EFLAGS: 00010217
RAX: dffffc0000000000 RBX: ffff8881148d7940 RCX: ffffffff817787bb
RDX: 0000000000000077 RSI: 0000000000000008 RDI: 00000000000003be
RBP: ffffc90000c9fb30 R08: 0000000000000000 R09: fffffbfff0d3e840
R10: ffffffff869f4207 R11: 0000000000000000 R12: 0000000000000000
R13: ffff888115eb9300 R14: ffffc90000c9fbc8 R15: 000000000000000c
FS:  0000000000000000(0000) GS:ffff8882b0151000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f37266b6114 CR3: 00000000054a8000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
 <TASK>
 unlock_ovpn+0x8b/0xe0 [ovpn]
 ovpn_peer_keepalive_work+0xe3/0x540 [ovpn]
 ? ovpn_peers_free+0x780/0x780 [ovpn]
 ? lock_acquire+0x56/0x70
 ? process_one_work+0x888/0x1740
 process_one_work+0x933/0x1740
 ? pwq_dec_nr_in_flight+0x10b0/0x10b0
 ? move_linked_works+0x12d/0x2c0
 ? assign_work+0x163/0x270
 worker_thread+0x4d6/0xd90
 ? preempt_count_sub+0x4c/0x70
 ? process_one_work+0x1740/0x1740
 kthread+0x36c/0x710
 ? trace_preempt_on+0x8c/0x1e0
 ? kthread_is_per_cpu+0xc0/0xc0
 ? preempt_count_sub+0x4c/0x70
 ? _raw_spin_unlock_irq+0x36/0x60
 ? calculate_sigpending+0x7b/0xa0
 ? kthread_is_per_cpu+0xc0/0xc0
 ret_from_fork+0x3a/0x80
 ? kthread_is_per_cpu+0xc0/0xc0
 ret_from_fork_asm+0x11/0x20
 </TASK>
Modules linked in: ovpn(O)

This happens because the peer deletion operation reaches
ovpn_socket_release() while ovpn_sock->sock (struct socket *)
and its sk member (struct sock *) are still both valid.
Here synchronize_rcu() is invoked, after which ovpn_sock->sock->sk
becomes NULL, due to the concurrent socket closing triggered
from userspace.

After having invoked synchronize_rcu(), ovpn_socket_release() will
attempt dereferencing ovpn_sock->sock->sk, triggering the crash
reported above.

The reason for accessing sk is that we need to retrieve its
protocol and continue the cleanup routine accordingly.

This crash can be easily produced by running openvpn userspace in
client mode with `--keepalive 10 20`, while entirely omitting this
option on the server side.
After 20 seconds ovpn will assume the peer (server) to be dead,
will start removing it and will notify userspace. The latter will
receive the notification and close the transport socket, thus
triggering the crash.

To fix the race condition for good, we need to refactor ovpn_socket.
Since we are always only interested in the sock->sk member (struct sock *)
we can directly hold a reference to it, raher than relying on
sock->sk.

This means changing ovpn_socket->sock to ovpn_socket->sk.

While holding a reference to sk, we can increase its refcounter
without affecting the socket close()/destroy() notification
(which we rely on when userspace closes a socket we are using).

By increasing sk's refcounter we know we can dereference it
in ovpn_socket_release() without incurring in any race condition
anymore.

ovpn_socket_release() will ultimately decrease the reference
counter.

Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Reported-by: Qingfang Deng <dqfext@gmail.com>
Closes: https://github.com/OpenVPN/ovpn-net-next/issues/1
Tested-by: Gert Doering <gert@greenie.muc.de>
Link: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31575.html
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c      |  8 ++---
 drivers/net/ovpn/netlink.c | 25 ++++++++------
 drivers/net/ovpn/peer.c    |  4 +--
 drivers/net/ovpn/socket.c  | 68 +++++++++++++++++++++-----------------
 drivers/net/ovpn/socket.h  |  4 +--
 drivers/net/ovpn/tcp.c     | 65 ++++++++++++++++++------------------
 drivers/net/ovpn/tcp.h     |  3 +-
 drivers/net/ovpn/udp.c     | 34 +++++++------------
 drivers/net/ovpn/udp.h     |  4 +--
 9 files changed, 109 insertions(+), 106 deletions(-)

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 10d8afecec55..ebf1e849506b 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -134,7 +134,7 @@ void ovpn_decrypt_post(void *data, int ret)
 
 	rcu_read_lock();
 	sock = rcu_dereference(peer->sock);
-	if (sock && sock->sock->sk->sk_protocol == IPPROTO_UDP)
+	if (sock && sock->sk->sk_protocol == IPPROTO_UDP)
 		/* check if this peer changed local or remote endpoint */
 		ovpn_peer_endpoints_update(peer, skb);
 	rcu_read_unlock();
@@ -270,12 +270,12 @@ void ovpn_encrypt_post(void *data, int ret)
 	if (unlikely(!sock))
 		goto err_unlock;
 
-	switch (sock->sock->sk->sk_protocol) {
+	switch (sock->sk->sk_protocol) {
 	case IPPROTO_UDP:
-		ovpn_udp_send_skb(peer, sock->sock, skb);
+		ovpn_udp_send_skb(peer, sock->sk, skb);
 		break;
 	case IPPROTO_TCP:
-		ovpn_tcp_send_skb(peer, sock->sock, skb);
+		ovpn_tcp_send_skb(peer, sock->sk, skb);
 		break;
 	default:
 		/* no transport configured yet */
diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index bea03913bfb1..1dd7e763c168 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -423,9 +423,14 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 	ovpn_sock = ovpn_socket_new(sock, peer);
 	/* at this point we unconditionally drop the reference to the socket:
 	 * - in case of error, the socket has to be dropped
-	 * - if case of success, the socket is configured and let
+	 * - if case of success, the socket is configured and we let
 	 *   userspace own the reference, so that the latter can
-	 *   trigger the final close()
+	 *   trigger the final close().
+	 *
+	 * NOTE: at this point ovpn_socket_new() has acquired a reference
+	 * to sock->sk. That's needed especially to avoid race conditions
+	 * during cleanup, where sock may still be alive, but sock->sk may be
+	 * getting released concurrently.
 	 */
 	sockfd_put(sock);
 	if (IS_ERR(ovpn_sock)) {
@@ -501,7 +506,7 @@ int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
 	/* when using a TCP socket the remote IP is not expected */
 	rcu_read_lock();
 	sock = rcu_dereference(peer->sock);
-	if (sock && sock->sock->sk->sk_protocol == IPPROTO_TCP &&
+	if (sock && sock->sk->sk_protocol == IPPROTO_TCP &&
 	    (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
 	     attrs[OVPN_A_PEER_REMOTE_IPV6])) {
 		rcu_read_unlock();
@@ -559,14 +564,14 @@ static int ovpn_nl_send_peer(struct sk_buff *skb, const struct genl_info *info,
 		goto err_unlock;
 	}
 
-	if (!net_eq(genl_info_net(info), sock_net(sock->sock->sk))) {
+	if (!net_eq(genl_info_net(info), sock_net(sock->sk))) {
 		id = peernet2id_alloc(genl_info_net(info),
-				      sock_net(sock->sock->sk),
+				      sock_net(sock->sk),
 				      GFP_ATOMIC);
 		if (nla_put_s32(skb, OVPN_A_PEER_SOCKET_NETNSID, id))
 			goto err_unlock;
 	}
-	local_port = inet_sk(sock->sock->sk)->inet_sport;
+	local_port = inet_sk(sock->sk)->inet_sport;
 	rcu_read_unlock();
 
 	if (nla_put_u32(skb, OVPN_A_PEER_ID, peer->id))
@@ -1153,8 +1158,8 @@ int ovpn_nl_peer_del_notify(struct ovpn_peer *peer)
 		ret = -EINVAL;
 		goto err_unlock;
 	}
-	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sock->sk),
-				msg, 0, OVPN_NLGRP_PEERS, GFP_ATOMIC);
+	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sk), msg, 0,
+				OVPN_NLGRP_PEERS, GFP_ATOMIC);
 	rcu_read_unlock();
 
 	return 0;
@@ -1218,8 +1223,8 @@ int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id)
 		ret = -EINVAL;
 		goto err_unlock;
 	}
-	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sock->sk),
-				msg, 0, OVPN_NLGRP_PEERS, GFP_ATOMIC);
+	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sk), msg, 0,
+				OVPN_NLGRP_PEERS, GFP_ATOMIC);
 	rcu_read_unlock();
 
 	return 0;
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a1fd27b9c038..4bfcab0c8652 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -1145,7 +1145,7 @@ static void ovpn_peer_release_p2p(struct ovpn_priv *ovpn, struct sock *sk,
 
 	if (sk) {
 		ovpn_sock = rcu_access_pointer(peer->sock);
-		if (!ovpn_sock || ovpn_sock->sock->sk != sk) {
+		if (!ovpn_sock || ovpn_sock->sk != sk) {
 			spin_unlock_bh(&ovpn->lock);
 			ovpn_peer_put(peer);
 			return;
@@ -1175,7 +1175,7 @@ static void ovpn_peers_release_mp(struct ovpn_priv *ovpn, struct sock *sk,
 		if (sk) {
 			rcu_read_lock();
 			ovpn_sock = rcu_dereference(peer->sock);
-			remove = ovpn_sock && ovpn_sock->sock->sk == sk;
+			remove = ovpn_sock && ovpn_sock->sk == sk;
 			rcu_read_unlock();
 		}
 
diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
index a83cbab72591..8c8c4f270f0d 100644
--- a/drivers/net/ovpn/socket.c
+++ b/drivers/net/ovpn/socket.c
@@ -24,9 +24,9 @@ static void ovpn_socket_release_kref(struct kref *kref)
 	struct ovpn_socket *sock = container_of(kref, struct ovpn_socket,
 						refcount);
 
-	if (sock->sock->sk->sk_protocol == IPPROTO_UDP)
+	if (sock->sk->sk_protocol == IPPROTO_UDP)
 		ovpn_udp_socket_detach(sock);
-	else if (sock->sock->sk->sk_protocol == IPPROTO_TCP)
+	else if (sock->sk->sk_protocol == IPPROTO_TCP)
 		ovpn_tcp_socket_detach(sock);
 }
 
@@ -75,14 +75,6 @@ void ovpn_socket_release(struct ovpn_peer *peer)
 	if (!sock)
 		return;
 
-	/* sanity check: we should not end up here if the socket
-	 * was already closed
-	 */
-	if (!sock->sock->sk) {
-		DEBUG_NET_WARN_ON_ONCE(1);
-		return;
-	}
-
 	/* Drop the reference while holding the sock lock to avoid
 	 * concurrent ovpn_socket_new call to mess up with a partially
 	 * detached socket.
@@ -90,22 +82,24 @@ void ovpn_socket_release(struct ovpn_peer *peer)
 	 * Holding the lock ensures that a socket with refcnt 0 is fully
 	 * detached before it can be picked by a concurrent reader.
 	 */
-	lock_sock(sock->sock->sk);
+	lock_sock(sock->sk);
 	released = ovpn_socket_put(peer, sock);
-	release_sock(sock->sock->sk);
+	release_sock(sock->sk);
 
 	/* align all readers with sk_user_data being NULL */
 	synchronize_rcu();
 
 	/* following cleanup should happen with lock released */
 	if (released) {
-		if (sock->sock->sk->sk_protocol == IPPROTO_UDP) {
+		if (sock->sk->sk_protocol == IPPROTO_UDP) {
 			netdev_put(sock->ovpn->dev, &sock->dev_tracker);
-		} else if (sock->sock->sk->sk_protocol == IPPROTO_TCP) {
+		} else if (sock->sk->sk_protocol == IPPROTO_TCP) {
 			/* wait for TCP jobs to terminate */
 			ovpn_tcp_socket_wait_finish(sock);
 			ovpn_peer_put(sock->peer);
 		}
+		/* drop reference acquired in ovpn_socket_new() */
+		sock_put(sock->sk);
 		/* we can call plain kfree() because we already waited one RCU
 		 * period due to synchronize_rcu()
 		 */
@@ -118,12 +112,14 @@ static bool ovpn_socket_hold(struct ovpn_socket *sock)
 	return kref_get_unless_zero(&sock->refcount);
 }
 
-static int ovpn_socket_attach(struct ovpn_socket *sock, struct ovpn_peer *peer)
+static int ovpn_socket_attach(struct ovpn_socket *ovpn_sock,
+			      struct socket *sock,
+			      struct ovpn_peer *peer)
 {
-	if (sock->sock->sk->sk_protocol == IPPROTO_UDP)
-		return ovpn_udp_socket_attach(sock, peer->ovpn);
-	else if (sock->sock->sk->sk_protocol == IPPROTO_TCP)
-		return ovpn_tcp_socket_attach(sock, peer);
+	if (sock->sk->sk_protocol == IPPROTO_UDP)
+		return ovpn_udp_socket_attach(ovpn_sock, sock, peer->ovpn);
+	else if (sock->sk->sk_protocol == IPPROTO_TCP)
+		return ovpn_tcp_socket_attach(ovpn_sock, peer);
 
 	return -EOPNOTSUPP;
 }
@@ -138,14 +134,15 @@ static int ovpn_socket_attach(struct ovpn_socket *sock, struct ovpn_peer *peer)
 struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 {
 	struct ovpn_socket *ovpn_sock;
+	struct sock *sk = sock->sk;
 	int ret;
 
-	lock_sock(sock->sk);
+	lock_sock(sk);
 
 	/* a TCP socket can only be owned by a single peer, therefore there
 	 * can't be any other user
 	 */
-	if (sock->sk->sk_protocol == IPPROTO_TCP && sock->sk->sk_user_data) {
+	if (sk->sk_protocol == IPPROTO_TCP && sk->sk_user_data) {
 		ovpn_sock = ERR_PTR(-EBUSY);
 		goto sock_release;
 	}
@@ -153,8 +150,8 @@ 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 (sock->sk->sk_protocol == IPPROTO_UDP) {
-		u8 type = READ_ONCE(udp_sk(sock->sk)->encap_type);
+	if (sk->sk_protocol == IPPROTO_UDP) {
+		u8 type = READ_ONCE(udp_sk(sk)->encap_type);
 
 		/* socket owned by other encapsulation module */
 		if (type && type != UDP_ENCAP_OVPNINUDP) {
@@ -163,7 +160,7 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 		}
 
 		rcu_read_lock();
-		ovpn_sock = rcu_dereference_sk_user_data(sock->sk);
+		ovpn_sock = rcu_dereference_sk_user_data(sk);
 		if (ovpn_sock) {
 			/* socket owned by another ovpn instance, we can't use it */
 			if (ovpn_sock->ovpn != peer->ovpn) {
@@ -192,19 +189,30 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 		rcu_read_unlock();
 	}
 
+	/* increase sk refcounter as we'll store a reference in
+	 * ovpn_socket.
+	 * ovpn_socket_release() will decrement the refcounter.
+	 */
+	if (!refcount_inc_not_zero(&sk->sk_refcnt)) {
+		ovpn_sock = ERR_PTR(-ENOTSOCK);
+		goto sock_release;
+	}
+
 	/* socket is not owned: attach to this ovpn instance */
 
 	ovpn_sock = kzalloc(sizeof(*ovpn_sock), GFP_KERNEL);
 	if (!ovpn_sock) {
+		sock_put(sk);
 		ovpn_sock = ERR_PTR(-ENOMEM);
 		goto sock_release;
 	}
 
-	ovpn_sock->sock = sock;
+	ovpn_sock->sk = sk;
 	kref_init(&ovpn_sock->refcount);
 
-	ret = ovpn_socket_attach(ovpn_sock, peer);
+	ret = ovpn_socket_attach(ovpn_sock, sock, peer);
 	if (ret < 0) {
+		sock_put(sk);
 		kfree(ovpn_sock);
 		ovpn_sock = ERR_PTR(ret);
 		goto sock_release;
@@ -213,11 +221,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 (sock->sk->sk_protocol == IPPROTO_TCP) {
+	if (sk->sk_protocol == IPPROTO_TCP) {
 		INIT_WORK(&ovpn_sock->tcp_tx_work, ovpn_tcp_tx_work);
 		ovpn_sock->peer = peer;
 		ovpn_peer_hold(peer);
-	} else if (sock->sk->sk_protocol == IPPROTO_UDP) {
+	} else if (sk->sk_protocol == IPPROTO_UDP) {
 		/* in UDP we only link the ovpn instance since the socket is
 		 * shared among multiple peers
 		 */
@@ -226,8 +234,8 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 			    GFP_KERNEL);
 	}
 
-	rcu_assign_sk_user_data(sock->sk, ovpn_sock);
+	rcu_assign_sk_user_data(sk, ovpn_sock);
 sock_release:
-	release_sock(sock->sk);
+	release_sock(sk);
 	return ovpn_sock;
 }
diff --git a/drivers/net/ovpn/socket.h b/drivers/net/ovpn/socket.h
index 00d856b1a5d8..4afcec71040d 100644
--- a/drivers/net/ovpn/socket.h
+++ b/drivers/net/ovpn/socket.h
@@ -22,7 +22,7 @@ struct ovpn_peer;
  * @ovpn: ovpn instance owning this socket (UDP only)
  * @dev_tracker: reference tracker for associated dev (UDP only)
  * @peer: unique peer transmitting over this socket (TCP only)
- * @sock: the low level sock object
+ * @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)
@@ -36,7 +36,7 @@ struct ovpn_socket {
 		struct ovpn_peer *peer;
 	};
 
-	struct socket *sock;
+	struct sock *sk;
 	struct kref refcount;
 	struct work_struct work;
 	struct work_struct tcp_tx_work;
diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 7c42d84987ad..7e79aad0b043 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -186,18 +186,18 @@ static int ovpn_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 void ovpn_tcp_socket_detach(struct ovpn_socket *ovpn_sock)
 {
 	struct ovpn_peer *peer = ovpn_sock->peer;
-	struct socket *sock = ovpn_sock->sock;
+	struct sock *sk = ovpn_sock->sk;
 
 	strp_stop(&peer->tcp.strp);
 	skb_queue_purge(&peer->tcp.user_queue);
 
 	/* restore CBs that were saved in ovpn_sock_set_tcp_cb() */
-	sock->sk->sk_data_ready = peer->tcp.sk_cb.sk_data_ready;
-	sock->sk->sk_write_space = peer->tcp.sk_cb.sk_write_space;
-	sock->sk->sk_prot = peer->tcp.sk_cb.prot;
-	sock->sk->sk_socket->ops = peer->tcp.sk_cb.ops;
+	sk->sk_data_ready = peer->tcp.sk_cb.sk_data_ready;
+	sk->sk_write_space = peer->tcp.sk_cb.sk_write_space;
+	sk->sk_prot = peer->tcp.sk_cb.prot;
+	sk->sk_socket->ops = peer->tcp.sk_cb.ops;
 
-	rcu_assign_sk_user_data(sock->sk, NULL);
+	rcu_assign_sk_user_data(sk, NULL);
 }
 
 void ovpn_tcp_socket_wait_finish(struct ovpn_socket *sock)
@@ -283,10 +283,10 @@ void ovpn_tcp_tx_work(struct work_struct *work)
 
 	sock = container_of(work, struct ovpn_socket, tcp_tx_work);
 
-	lock_sock(sock->sock->sk);
+	lock_sock(sock->sk);
 	if (sock->peer)
-		ovpn_tcp_send_sock(sock->peer, sock->sock->sk);
-	release_sock(sock->sock->sk);
+		ovpn_tcp_send_sock(sock->peer, sock->sk);
+	release_sock(sock->sk);
 }
 
 static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
@@ -307,15 +307,15 @@ static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
 	ovpn_tcp_send_sock(peer, sk);
 }
 
-void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct socket *sock,
+void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 		       struct sk_buff *skb)
 {
 	u16 len = skb->len;
 
 	*(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);
 
-	spin_lock_nested(&sock->sk->sk_lock.slock, OVPN_TCP_DEPTH_NESTING);
-	if (sock_owned_by_user(sock->sk)) {
+	spin_lock_nested(&sk->sk_lock.slock, OVPN_TCP_DEPTH_NESTING);
+	if (sock_owned_by_user(sk)) {
 		if (skb_queue_len(&peer->tcp.out_queue) >=
 		    READ_ONCE(net_hotdata.max_backlog)) {
 			dev_dstats_tx_dropped(peer->ovpn->dev);
@@ -324,10 +324,10 @@ void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct socket *sock,
 		}
 		__skb_queue_tail(&peer->tcp.out_queue, skb);
 	} else {
-		ovpn_tcp_send_sock_skb(peer, sock->sk, skb);
+		ovpn_tcp_send_sock_skb(peer, sk, skb);
 	}
 unlock:
-	spin_unlock(&sock->sk->sk_lock.slock);
+	spin_unlock(&sk->sk_lock.slock);
 }
 
 static void ovpn_tcp_release(struct sock *sk)
@@ -474,7 +474,6 @@ static void ovpn_tcp_peer_del_work(struct work_struct *work)
 int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 			   struct ovpn_peer *peer)
 {
-	struct socket *sock = ovpn_sock->sock;
 	struct strp_callbacks cb = {
 		.rcv_msg = ovpn_tcp_rcv,
 		.parse_msg = ovpn_tcp_parse,
@@ -482,20 +481,20 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 	int ret;
 
 	/* make sure no pre-existing encapsulation handler exists */
-	if (sock->sk->sk_user_data)
+	if (ovpn_sock->sk->sk_user_data)
 		return -EBUSY;
 
 	/* only a fully connected socket is expected. Connection should be
 	 * handled in userspace
 	 */
-	if (sock->sk->sk_state != TCP_ESTABLISHED) {
+	if (ovpn_sock->sk->sk_state != TCP_ESTABLISHED) {
 		net_err_ratelimited("%s: provided TCP socket is not in ESTABLISHED state: %d\n",
 				    netdev_name(peer->ovpn->dev),
-				    sock->sk->sk_state);
+				    ovpn_sock->sk->sk_state);
 		return -EINVAL;
 	}
 
-	ret = strp_init(&peer->tcp.strp, sock->sk, &cb);
+	ret = strp_init(&peer->tcp.strp, ovpn_sock->sk, &cb);
 	if (ret < 0) {
 		DEBUG_NET_WARN_ON_ONCE(1);
 		return ret;
@@ -503,31 +502,31 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 
 	INIT_WORK(&peer->tcp.defer_del_work, ovpn_tcp_peer_del_work);
 
-	__sk_dst_reset(sock->sk);
+	__sk_dst_reset(ovpn_sock->sk);
 	skb_queue_head_init(&peer->tcp.user_queue);
 	skb_queue_head_init(&peer->tcp.out_queue);
 
 	/* save current CBs so that they can be restored upon socket release */
-	peer->tcp.sk_cb.sk_data_ready = sock->sk->sk_data_ready;
-	peer->tcp.sk_cb.sk_write_space = sock->sk->sk_write_space;
-	peer->tcp.sk_cb.prot = sock->sk->sk_prot;
-	peer->tcp.sk_cb.ops = sock->sk->sk_socket->ops;
+	peer->tcp.sk_cb.sk_data_ready = ovpn_sock->sk->sk_data_ready;
+	peer->tcp.sk_cb.sk_write_space = ovpn_sock->sk->sk_write_space;
+	peer->tcp.sk_cb.prot = ovpn_sock->sk->sk_prot;
+	peer->tcp.sk_cb.ops = ovpn_sock->sk->sk_socket->ops;
 
 	/* assign our static CBs and prot/ops */
-	sock->sk->sk_data_ready = ovpn_tcp_data_ready;
-	sock->sk->sk_write_space = ovpn_tcp_write_space;
+	ovpn_sock->sk->sk_data_ready = ovpn_tcp_data_ready;
+	ovpn_sock->sk->sk_write_space = ovpn_tcp_write_space;
 
-	if (sock->sk->sk_family == AF_INET) {
-		sock->sk->sk_prot = &ovpn_tcp_prot;
-		sock->sk->sk_socket->ops = &ovpn_tcp_ops;
+	if (ovpn_sock->sk->sk_family == AF_INET) {
+		ovpn_sock->sk->sk_prot = &ovpn_tcp_prot;
+		ovpn_sock->sk->sk_socket->ops = &ovpn_tcp_ops;
 	} else {
-		sock->sk->sk_prot = &ovpn_tcp6_prot;
-		sock->sk->sk_socket->ops = &ovpn_tcp6_ops;
+		ovpn_sock->sk->sk_prot = &ovpn_tcp6_prot;
+		ovpn_sock->sk->sk_socket->ops = &ovpn_tcp6_ops;
 	}
 
 	/* avoid using task_frag */
-	sock->sk->sk_allocation = GFP_ATOMIC;
-	sock->sk->sk_use_task_frag = false;
+	ovpn_sock->sk->sk_allocation = GFP_ATOMIC;
+	ovpn_sock->sk->sk_use_task_frag = false;
 
 	/* enqueue the RX worker */
 	strp_check_rcv(&peer->tcp.strp);
diff --git a/drivers/net/ovpn/tcp.h b/drivers/net/ovpn/tcp.h
index 10aefa834cf3..a3aa3570ae5e 100644
--- a/drivers/net/ovpn/tcp.h
+++ b/drivers/net/ovpn/tcp.h
@@ -30,7 +30,8 @@ void ovpn_tcp_socket_wait_finish(struct ovpn_socket *sock);
  * 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 socket *sock, struct sk_buff *skb);
+void 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_ */
diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index c8a81c4d6489..b4fbebad8f45 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -43,7 +43,7 @@ static struct ovpn_socket *ovpn_socket_from_udp_sock(struct sock *sk)
 		return NULL;
 
 	/* make sure that sk matches our stored transport socket */
-	if (unlikely(!ovpn_sock->sock || sk != ovpn_sock->sock->sk))
+	if (unlikely(!ovpn_sock->sk || sk != ovpn_sock->sk))
 		return NULL;
 
 	return ovpn_sock;
@@ -335,32 +335,22 @@ static int ovpn_udp_output(struct ovpn_peer *peer, struct dst_cache *cache,
 /**
  * ovpn_udp_send_skb - prepare skb and send it over via UDP
  * @peer: the destination peer
- * @sock: the RCU protected peer socket
+ * @sk: peer socket
  * @skb: the packet to send
  */
-void ovpn_udp_send_skb(struct ovpn_peer *peer, struct socket *sock,
+void ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 		       struct sk_buff *skb)
 {
-	int ret = -1;
+	int ret;
 
 	skb->dev = peer->ovpn->dev;
 	/* no checksum performed at this layer */
 	skb->ip_summed = CHECKSUM_NONE;
 
-	/* get socket info */
-	if (unlikely(!sock)) {
-		net_warn_ratelimited("%s: no sock for remote peer %u\n",
-				     netdev_name(peer->ovpn->dev), peer->id);
-		goto out;
-	}
-
 	/* crypto layer -> transport (UDP) */
-	ret = ovpn_udp_output(peer, &peer->dst_cache, sock->sk, skb);
-out:
-	if (unlikely(ret < 0)) {
+	ret = ovpn_udp_output(peer, &peer->dst_cache, sk, skb);
+	if (unlikely(ret < 0))
 		kfree_skb(skb);
-		return;
-	}
 }
 
 static void ovpn_udp_encap_destroy(struct sock *sk)
@@ -383,6 +373,7 @@ static void ovpn_udp_encap_destroy(struct sock *sk)
 /**
  * ovpn_udp_socket_attach - set udp-tunnel CBs on socket and link it to ovpn
  * @ovpn_sock: socket to configure
+ * @sock: the socket container to be passed to setup_udp_tunnel_sock()
  * @ovpn: the openvp instance to link
  *
  * After invoking this function, the sock will be controlled by ovpn so that
@@ -390,7 +381,7 @@ static void ovpn_udp_encap_destroy(struct sock *sk)
  *
  * Return: 0 on success or a negative error code otherwise
  */
-int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
+int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock, struct socket *sock,
 			   struct ovpn_priv *ovpn)
 {
 	struct udp_tunnel_sock_cfg cfg = {
@@ -398,17 +389,16 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
 		.encap_rcv = ovpn_udp_encap_recv,
 		.encap_destroy = ovpn_udp_encap_destroy,
 	};
-	struct socket *sock = ovpn_sock->sock;
 	struct ovpn_socket *old_data;
 	int ret;
 
 	/* make sure no pre-existing encapsulation handler exists */
 	rcu_read_lock();
-	old_data = rcu_dereference_sk_user_data(sock->sk);
+	old_data = rcu_dereference_sk_user_data(ovpn_sock->sk);
 	if (!old_data) {
 		/* socket is currently unused - we can take it */
 		rcu_read_unlock();
-		setup_udp_tunnel_sock(sock_net(sock->sk), sock, &cfg);
+		setup_udp_tunnel_sock(sock_net(ovpn_sock->sk), sock, &cfg);
 		return 0;
 	}
 
@@ -421,7 +411,7 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
 	 * Unlikely TCP, a single UDP socket can be used to talk to many remote
 	 * hosts and therefore openvpn instantiates one only for all its peers
 	 */
-	if ((READ_ONCE(udp_sk(sock->sk)->encap_type) == UDP_ENCAP_OVPNINUDP) &&
+	if ((READ_ONCE(udp_sk(ovpn_sock->sk)->encap_type) == UDP_ENCAP_OVPNINUDP) &&
 	    old_data->ovpn == ovpn) {
 		netdev_dbg(ovpn->dev,
 			   "provided socket already owned by this interface\n");
@@ -442,5 +432,5 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
  */
 void ovpn_udp_socket_detach(struct ovpn_socket *ovpn_sock)
 {
-	cleanup_udp_tunnel_sock(ovpn_sock->sock->sk);
+	cleanup_udp_tunnel_sock(ovpn_sock->sk);
 }
diff --git a/drivers/net/ovpn/udp.h b/drivers/net/ovpn/udp.h
index 9994eb6e0428..fe26fbe25c5a 100644
--- a/drivers/net/ovpn/udp.h
+++ b/drivers/net/ovpn/udp.h
@@ -15,11 +15,11 @@ struct ovpn_peer;
 struct ovpn_priv;
 struct socket;
 
-int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
+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 socket *sock,
+void ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 		       struct sk_buff *skb);
 
 #endif /* _NET_OVPN_UDP_H_ */
-- 
2.49.0

[PATCH net-next 0/4] pull request: fixes for ovpn 2025-05-27

[Antonio Quartulli]

Hi netdev-team,
I am targetting net-next because ovpn has been merged to net yet.

In this batch you can find two major bugfixes:

Patch 1: when releasing a UDP socket we were wrongly invoking
setup_udp_tunnel_sock() with an empty config. This was not
properly shutting down the UDP encap state.
This patch fixes this misbehaviour by implementing an "undo"
function called cleanup_udp_tunnel_sock() which properly
takes care or unwinding the encap state.
Implementing this function required some extra changes to the
udp/udp_tunnel code, especially to avoid race conditions with
udp_destroy_sock().

Patch 2: ovpn was holding a reference to a 'struct socket'
without increasing its reference counter. This was intended
and worked as expected until we hit a race condition where
user space tries to close the socket while kernel space is
also releasing it. In this case the (struct socket *)->sk
member would disappear under our feet leading to a null-ptr-deref.
This patch fixes this issue by having ovpn hold a reference
directly to the sk member while also increasing its reference
counter.

Patch 3 and 4 are instead fixing minor issues in the ovpn
kselftests.


Please pull.


Thanks a lot,
Antonio


The following changes since commit b2908a989c594f9eb1c93016abc1382f97ee02b1:

  net: phy: add driver for MaxLinear MxL86110 PHY (2025-05-27 09:36:17 +0200)

are available in the Git repository at:

  https://github.com/OpenVPN/ovpn-net-next tags/ovpn-net-next-20250527

for you to fetch changes up to 545e4c173cf94b4e5bbfe8ee09de2fbe556bb75a:

  selftest/net/ovpn: fix missing file (2025-05-27 13:50:50 +0200)

----------------------------------------------------------------
This bugfix batch includes the following changes:
* dropped call to setup_udp_tunnel_sock() during cleanup,
  substituted by new cleanup_udp_tunnel_sock(), which
  required:
** implementing udp_tunnel_encap_disable()
** implementing udpv6_encap_disable()
** calling udp_test_and_clear_bit() in udp_destroy_socket()
* fixed race condition between peer removal (by kernel
  space) and socket closing (by user space)
* fixes for ovpn kselftests

----------------------------------------------------------------
Antonio Quartulli (4):
      ovpn: properly deconfigure UDP-tunnel
      ovpn: ensure sk is still valid during cleanup
      selftest/net/ovpn: fix TCP socket creation
      selftest/net/ovpn: fix missing file

 drivers/net/ovpn/io.c                              |  8 +--
 drivers/net/ovpn/netlink.c                         | 25 ++++----
 drivers/net/ovpn/peer.c                            |  4 +-
 drivers/net/ovpn/socket.c                          | 68 ++++++++++++----------
 drivers/net/ovpn/socket.h                          |  4 +-
 drivers/net/ovpn/tcp.c                             | 65 ++++++++++-----------
 drivers/net/ovpn/tcp.h                             |  3 +-
 drivers/net/ovpn/udp.c                             | 37 ++++--------
 drivers/net/ovpn/udp.h                             |  4 +-
 include/linux/udp.h                                |  2 +
 include/net/ipv6_stubs.h                           |  1 +
 include/net/udp.h                                  |  1 +
 include/net/udp_tunnel.h                           | 13 +++++
 net/ipv4/udp.c                                     |  2 +-
 net/ipv4/udp_tunnel_core.c                         | 22 +++++++
 net/ipv6/af_inet6.c                                |  1 +
 net/ipv6/udp.c                                     |  8 ++-
 tools/testing/selftests/net/ovpn/ovpn-cli.c        |  1 +
 tools/testing/selftests/net/ovpn/test-large-mtu.sh |  9 +++
 19 files changed, 167 insertions(+), 111 deletions(-)
 create mode 100755 tools/testing/selftests/net/ovpn/test-large-mtu.sh

[PATCH net-next 1/4] ovpn: properly deconfigure UDP-tunnel

[Antonio Quartulli]

When deconfiguring a UDP-tunnel from a socket, we cannot
call setup_udp_tunnel_sock(), as it is expected to be invoked
only during setup.

Implement a new function named cleanup_udp_tunnel_sock(),
that reverts was what done during setup, and invoke it.

This new function takes care of reverting everything that
was done by setup_udp_tunnel_sock() (i.e. unsetting various
members and cleaning up the encap state in the kernel).

Note that cleanup_udp_tunnel_sock() takes 'struct sock'
as argument in preparation for a follow up patch, where
'struct ovpn' won't hold any reference to any 'struct socket'
any more, but rather to its 'struct sock' member only.

To properly implement cleanup_udp_tunnel_sock(), two extra
items were required:

1) implement udpv6_encap_disable() in order to
allow udpv6_encap_needed_key to be decreased outside of
ipv6.ko (similarly to udp_encap_disable() for IPv4).

2) avoid race conditions with udp_destroy_socket() by
atomically testing-and-clearing the ENCAP_ENABLED bit
on the socket before decreasing udp_encap_needed_key.
This way we prevent accidental double decreses.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Simon Horman <horms@kernel.org>
Cc: David Ahern <dsahern@kernel.org>
Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
Fixes: ab66abbc769b ("ovpn: implement basic RX path (UDP)")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Closes: https://lore.kernel.org/netdev/1a47ce02-fd42-4761-8697-f3f315011cc6@redhat.com
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/udp.c     |  5 +----
 include/linux/udp.h        |  2 ++
 include/net/ipv6_stubs.h   |  1 +
 include/net/udp.h          |  1 +
 include/net/udp_tunnel.h   | 13 +++++++++++++
 net/ipv4/udp.c             |  2 +-
 net/ipv4/udp_tunnel_core.c | 22 ++++++++++++++++++++++
 net/ipv6/af_inet6.c        |  1 +
 net/ipv6/udp.c             |  8 +++++++-
 9 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index aef8c0406ec9..c8a81c4d6489 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -442,8 +442,5 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
  */
 void ovpn_udp_socket_detach(struct ovpn_socket *ovpn_sock)
 {
-	struct udp_tunnel_sock_cfg cfg = { };
-
-	setup_udp_tunnel_sock(sock_net(ovpn_sock->sock->sk), ovpn_sock->sock,
-			      &cfg);
+	cleanup_udp_tunnel_sock(ovpn_sock->sock->sk);
 }
diff --git a/include/linux/udp.h b/include/linux/udp.h
index 4e1a672af4c5..97151f25953d 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -116,6 +116,8 @@ struct udp_sock {
 	set_bit(UDP_FLAGS_##nr, &udp_sk(sk)->udp_flags)
 #define udp_test_and_set_bit(nr, sk)		\
 	test_and_set_bit(UDP_FLAGS_##nr, &udp_sk(sk)->udp_flags)
+#define udp_test_and_clear_bit(nr, sk)		\
+	test_and_clear_bit(UDP_FLAGS_##nr, &udp_sk(sk)->udp_flags)
 #define udp_clear_bit(nr, sk)			\
 	clear_bit(UDP_FLAGS_##nr, &udp_sk(sk)->udp_flags)
 #define udp_assign_bit(nr, sk, val)		\
diff --git a/include/net/ipv6_stubs.h b/include/net/ipv6_stubs.h
index 8a3465c8c2c5..2a57df87d11b 100644
--- a/include/net/ipv6_stubs.h
+++ b/include/net/ipv6_stubs.h
@@ -55,6 +55,7 @@ struct ipv6_stub {
 			       struct nl_info *info);
 
 	void (*udpv6_encap_enable)(void);
+	void (*udpv6_encap_disable)(void);
 	void (*ndisc_send_na)(struct net_device *dev, const struct in6_addr *daddr,
 			      const struct in6_addr *solicited_addr,
 			      bool router, bool solicited, bool override, bool inc_opt);
diff --git a/include/net/udp.h b/include/net/udp.h
index a772510b2aa5..e3d7b8622f59 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -580,6 +580,7 @@ void udp_encap_disable(void);
 #if IS_ENABLED(CONFIG_IPV6)
 DECLARE_STATIC_KEY_FALSE(udpv6_encap_needed_key);
 void udpv6_encap_enable(void);
+void udpv6_encap_disable(void);
 #endif
 
 static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index 2df3b8344eb5..a14b171e7dd5 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -92,6 +92,7 @@ struct udp_tunnel_sock_cfg {
 /* Setup the given (UDP) sock to receive UDP encapsulated packets */
 void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
 			   struct udp_tunnel_sock_cfg *sock_cfg);
+void cleanup_udp_tunnel_sock(struct sock *sk);
 
 /* -- List of parsable UDP tunnel types --
  *
@@ -218,6 +219,18 @@ static inline void udp_tunnel_encap_enable(struct sock *sk)
 	udp_encap_enable();
 }
 
+static inline void udp_tunnel_encap_disable(struct sock *sk)
+{
+	if (!udp_test_and_clear_bit(ENCAP_ENABLED, sk))
+		return;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (READ_ONCE(sk->sk_family) == PF_INET6)
+		ipv6_stub->udpv6_encap_disable();
+#endif
+	udp_encap_disable();
+}
+
 #define UDP_TUNNEL_NIC_MAX_TABLES	4
 
 enum udp_tunnel_nic_info_flags {
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index dde52b8050b8..9ffc4e0b1644 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2900,7 +2900,7 @@ void udp_destroy_sock(struct sock *sk)
 			if (encap_destroy)
 				encap_destroy(sk);
 		}
-		if (udp_test_bit(ENCAP_ENABLED, sk)) {
+		if (udp_test_and_clear_bit(ENCAP_ENABLED, sk)) {
 			static_branch_dec(&udp_encap_needed_key);
 			udp_tunnel_cleanup_gro(sk);
 		}
diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
index 2326548997d3..624b6afcf812 100644
--- a/net/ipv4/udp_tunnel_core.c
+++ b/net/ipv4/udp_tunnel_core.c
@@ -98,6 +98,28 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
 }
 EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
 
+void cleanup_udp_tunnel_sock(struct sock *sk)
+{
+	/* Re-enable multicast loopback */
+	inet_set_bit(MC_LOOP, sk);
+
+	/* Disable CHECKSUM_UNNECESSARY to CHECKSUM_COMPLETE conversion */
+	inet_dec_convert_csum(sk);
+
+	udp_sk(sk)->encap_type = 0;
+	udp_sk(sk)->encap_rcv = NULL;
+	udp_sk(sk)->encap_err_rcv = NULL;
+	udp_sk(sk)->encap_err_lookup = NULL;
+	udp_sk(sk)->encap_destroy = NULL;
+	udp_sk(sk)->gro_receive = NULL;
+	udp_sk(sk)->gro_complete = NULL;
+
+	rcu_assign_sk_user_data(sk, NULL);
+	udp_tunnel_encap_disable(sk);
+	udp_tunnel_cleanup_gro(sk);
+}
+EXPORT_SYMBOL_GPL(cleanup_udp_tunnel_sock);
+
 void udp_tunnel_push_rx_port(struct net_device *dev, struct socket *sock,
 			     unsigned short type)
 {
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index acaff1296783..f3745705a811 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -1049,6 +1049,7 @@ static const struct ipv6_stub ipv6_stub_impl = {
 	.fib6_rt_update	   = fib6_rt_update,
 	.ip6_del_rt	   = ip6_del_rt,
 	.udpv6_encap_enable = udpv6_encap_enable,
+	.udpv6_encap_disable = udpv6_encap_disable,
 	.ndisc_send_na = ndisc_send_na,
 #if IS_ENABLED(CONFIG_XFRM)
 	.xfrm6_local_rxpmtu = xfrm6_local_rxpmtu,
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 7317f8e053f1..a919458772d6 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -602,6 +602,12 @@ void udpv6_encap_enable(void)
 }
 EXPORT_SYMBOL(udpv6_encap_enable);
 
+void udpv6_encap_disable(void)
+{
+	static_branch_dec(&udpv6_encap_needed_key);
+}
+EXPORT_SYMBOL(udpv6_encap_disable);
+
 /* Handler for tunnels with arbitrary destination ports: no socket lookup, go
  * through error handlers in encapsulations looking for a match.
  */
@@ -1823,7 +1829,7 @@ void udpv6_destroy_sock(struct sock *sk)
 			if (encap_destroy)
 				encap_destroy(sk);
 		}
-		if (udp_test_bit(ENCAP_ENABLED, sk)) {
+		if (udp_test_and_clear_bit(ENCAP_ENABLED, sk)) {
 			static_branch_dec(&udpv6_encap_needed_key);
 			udp_encap_disable();
 			udp_tunnel_cleanup_gro(sk);
-- 
2.49.0

[PATCH net-next 2/4] ovpn: ensure sk is still valid during cleanup

[Antonio Quartulli]

Removing a peer while userspace attempts to close its transport
socket triggers a race condition resulting in the following
crash:

Oops: general protection fault, probably for non-canonical address 0xdffffc0000000077: 0000 [#1] SMP KASAN
KASAN: null-ptr-deref in range [0x00000000000003b8-0x00000000000003bf]
CPU: 12 UID: 0 PID: 162 Comm: kworker/12:1 Tainted: G           O        6.15.0-rc2-00635-g521139ac3840 #272 PREEMPT(full)
Tainted: [O]=OOT_MODULE
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-20240910_120124-localhost 04/01/2014
Workqueue: events ovpn_peer_keepalive_work [ovpn]
RIP: 0010:ovpn_socket_release+0x23c/0x500 [ovpn]
Code: ea 03 80 3c 02 00 0f 85 71 02 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 64 24 18 49 8d bc 24 be 03 00 00 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 01 38 d0 7c 08 84 d2 0f 85 30
RSP: 0018:ffffc90000c9fb18 EFLAGS: 00010217
RAX: dffffc0000000000 RBX: ffff8881148d7940 RCX: ffffffff817787bb
RDX: 0000000000000077 RSI: 0000000000000008 RDI: 00000000000003be
RBP: ffffc90000c9fb30 R08: 0000000000000000 R09: fffffbfff0d3e840
R10: ffffffff869f4207 R11: 0000000000000000 R12: 0000000000000000
R13: ffff888115eb9300 R14: ffffc90000c9fbc8 R15: 000000000000000c
FS:  0000000000000000(0000) GS:ffff8882b0151000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f37266b6114 CR3: 00000000054a8000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
 <TASK>
 unlock_ovpn+0x8b/0xe0 [ovpn]
 ovpn_peer_keepalive_work+0xe3/0x540 [ovpn]
 ? ovpn_peers_free+0x780/0x780 [ovpn]
 ? lock_acquire+0x56/0x70
 ? process_one_work+0x888/0x1740
 process_one_work+0x933/0x1740
 ? pwq_dec_nr_in_flight+0x10b0/0x10b0
 ? move_linked_works+0x12d/0x2c0
 ? assign_work+0x163/0x270
 worker_thread+0x4d6/0xd90
 ? preempt_count_sub+0x4c/0x70
 ? process_one_work+0x1740/0x1740
 kthread+0x36c/0x710
 ? trace_preempt_on+0x8c/0x1e0
 ? kthread_is_per_cpu+0xc0/0xc0
 ? preempt_count_sub+0x4c/0x70
 ? _raw_spin_unlock_irq+0x36/0x60
 ? calculate_sigpending+0x7b/0xa0
 ? kthread_is_per_cpu+0xc0/0xc0
 ret_from_fork+0x3a/0x80
 ? kthread_is_per_cpu+0xc0/0xc0
 ret_from_fork_asm+0x11/0x20
 </TASK>
Modules linked in: ovpn(O)

This happens because the peer deletion operation reaches
ovpn_socket_release() while ovpn_sock->sock (struct socket *)
and its sk member (struct sock *) are still both valid.
Here synchronize_rcu() is invoked, after which ovpn_sock->sock->sk
becomes NULL, due to the concurrent socket closing triggered
from userspace.

After having invoked synchronize_rcu(), ovpn_socket_release() will
attempt dereferencing ovpn_sock->sock->sk, triggering the crash
reported above.

The reason for accessing sk is that we need to retrieve its
protocol and continue the cleanup routine accordingly.

This crash can be easily produced by running openvpn userspace in
client mode with `--keepalive 10 20`, while entirely omitting this
option on the server side.
After 20 seconds ovpn will assume the peer (server) to be dead,
will start removing it and will notify userspace. The latter will
receive the notification and close the transport socket, thus
triggering the crash.

To fix the race condition for good, we need to refactor struct ovpn_socket.
Since ovpn is always only interested in the sock->sk member (struct sock *)
we can directly hold a reference to it, raher than accessing it via
its struct socket container.

This means changing "struct socket *ovpn_socket->sock" to
"struct sock *ovpn_socket->sk".

While acquiring a reference to sk, we can increase its refcounter
without affecting the socket close()/destroy() notification
(which we rely on when userspace closes a socket we are using).

By increasing sk's refcounter we know we can dereference it
in ovpn_socket_release() without incurring in any race condition
anymore.

ovpn_socket_release() will ultimately decrease the reference
counter.

Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Reported-by: Qingfang Deng <dqfext@gmail.com>
Closes: https://github.com/OpenVPN/ovpn-net-next/issues/1
Tested-by: Gert Doering <gert@greenie.muc.de>
Link: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31575.html
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c      |  8 ++---
 drivers/net/ovpn/netlink.c | 25 ++++++++------
 drivers/net/ovpn/peer.c    |  4 +--
 drivers/net/ovpn/socket.c  | 68 +++++++++++++++++++++-----------------
 drivers/net/ovpn/socket.h  |  4 +--
 drivers/net/ovpn/tcp.c     | 65 ++++++++++++++++++------------------
 drivers/net/ovpn/tcp.h     |  3 +-
 drivers/net/ovpn/udp.c     | 34 +++++++------------
 drivers/net/ovpn/udp.h     |  4 +--
 9 files changed, 109 insertions(+), 106 deletions(-)

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 10d8afecec55..ebf1e849506b 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -134,7 +134,7 @@ void ovpn_decrypt_post(void *data, int ret)
 
 	rcu_read_lock();
 	sock = rcu_dereference(peer->sock);
-	if (sock && sock->sock->sk->sk_protocol == IPPROTO_UDP)
+	if (sock && sock->sk->sk_protocol == IPPROTO_UDP)
 		/* check if this peer changed local or remote endpoint */
 		ovpn_peer_endpoints_update(peer, skb);
 	rcu_read_unlock();
@@ -270,12 +270,12 @@ void ovpn_encrypt_post(void *data, int ret)
 	if (unlikely(!sock))
 		goto err_unlock;
 
-	switch (sock->sock->sk->sk_protocol) {
+	switch (sock->sk->sk_protocol) {
 	case IPPROTO_UDP:
-		ovpn_udp_send_skb(peer, sock->sock, skb);
+		ovpn_udp_send_skb(peer, sock->sk, skb);
 		break;
 	case IPPROTO_TCP:
-		ovpn_tcp_send_skb(peer, sock->sock, skb);
+		ovpn_tcp_send_skb(peer, sock->sk, skb);
 		break;
 	default:
 		/* no transport configured yet */
diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index bea03913bfb1..1dd7e763c168 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -423,9 +423,14 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 	ovpn_sock = ovpn_socket_new(sock, peer);
 	/* at this point we unconditionally drop the reference to the socket:
 	 * - in case of error, the socket has to be dropped
-	 * - if case of success, the socket is configured and let
+	 * - if case of success, the socket is configured and we let
 	 *   userspace own the reference, so that the latter can
-	 *   trigger the final close()
+	 *   trigger the final close().
+	 *
+	 * NOTE: at this point ovpn_socket_new() has acquired a reference
+	 * to sock->sk. That's needed especially to avoid race conditions
+	 * during cleanup, where sock may still be alive, but sock->sk may be
+	 * getting released concurrently.
 	 */
 	sockfd_put(sock);
 	if (IS_ERR(ovpn_sock)) {
@@ -501,7 +506,7 @@ int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
 	/* when using a TCP socket the remote IP is not expected */
 	rcu_read_lock();
 	sock = rcu_dereference(peer->sock);
-	if (sock && sock->sock->sk->sk_protocol == IPPROTO_TCP &&
+	if (sock && sock->sk->sk_protocol == IPPROTO_TCP &&
 	    (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
 	     attrs[OVPN_A_PEER_REMOTE_IPV6])) {
 		rcu_read_unlock();
@@ -559,14 +564,14 @@ static int ovpn_nl_send_peer(struct sk_buff *skb, const struct genl_info *info,
 		goto err_unlock;
 	}
 
-	if (!net_eq(genl_info_net(info), sock_net(sock->sock->sk))) {
+	if (!net_eq(genl_info_net(info), sock_net(sock->sk))) {
 		id = peernet2id_alloc(genl_info_net(info),
-				      sock_net(sock->sock->sk),
+				      sock_net(sock->sk),
 				      GFP_ATOMIC);
 		if (nla_put_s32(skb, OVPN_A_PEER_SOCKET_NETNSID, id))
 			goto err_unlock;
 	}
-	local_port = inet_sk(sock->sock->sk)->inet_sport;
+	local_port = inet_sk(sock->sk)->inet_sport;
 	rcu_read_unlock();
 
 	if (nla_put_u32(skb, OVPN_A_PEER_ID, peer->id))
@@ -1153,8 +1158,8 @@ int ovpn_nl_peer_del_notify(struct ovpn_peer *peer)
 		ret = -EINVAL;
 		goto err_unlock;
 	}
-	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sock->sk),
-				msg, 0, OVPN_NLGRP_PEERS, GFP_ATOMIC);
+	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sk), msg, 0,
+				OVPN_NLGRP_PEERS, GFP_ATOMIC);
 	rcu_read_unlock();
 
 	return 0;
@@ -1218,8 +1223,8 @@ int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id)
 		ret = -EINVAL;
 		goto err_unlock;
 	}
-	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sock->sk),
-				msg, 0, OVPN_NLGRP_PEERS, GFP_ATOMIC);
+	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sk), msg, 0,
+				OVPN_NLGRP_PEERS, GFP_ATOMIC);
 	rcu_read_unlock();
 
 	return 0;
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a1fd27b9c038..4bfcab0c8652 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -1145,7 +1145,7 @@ static void ovpn_peer_release_p2p(struct ovpn_priv *ovpn, struct sock *sk,
 
 	if (sk) {
 		ovpn_sock = rcu_access_pointer(peer->sock);
-		if (!ovpn_sock || ovpn_sock->sock->sk != sk) {
+		if (!ovpn_sock || ovpn_sock->sk != sk) {
 			spin_unlock_bh(&ovpn->lock);
 			ovpn_peer_put(peer);
 			return;
@@ -1175,7 +1175,7 @@ static void ovpn_peers_release_mp(struct ovpn_priv *ovpn, struct sock *sk,
 		if (sk) {
 			rcu_read_lock();
 			ovpn_sock = rcu_dereference(peer->sock);
-			remove = ovpn_sock && ovpn_sock->sock->sk == sk;
+			remove = ovpn_sock && ovpn_sock->sk == sk;
 			rcu_read_unlock();
 		}
 
diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
index a83cbab72591..8c8c4f270f0d 100644
--- a/drivers/net/ovpn/socket.c
+++ b/drivers/net/ovpn/socket.c
@@ -24,9 +24,9 @@ static void ovpn_socket_release_kref(struct kref *kref)
 	struct ovpn_socket *sock = container_of(kref, struct ovpn_socket,
 						refcount);
 
-	if (sock->sock->sk->sk_protocol == IPPROTO_UDP)
+	if (sock->sk->sk_protocol == IPPROTO_UDP)
 		ovpn_udp_socket_detach(sock);
-	else if (sock->sock->sk->sk_protocol == IPPROTO_TCP)
+	else if (sock->sk->sk_protocol == IPPROTO_TCP)
 		ovpn_tcp_socket_detach(sock);
 }
 
@@ -75,14 +75,6 @@ void ovpn_socket_release(struct ovpn_peer *peer)
 	if (!sock)
 		return;
 
-	/* sanity check: we should not end up here if the socket
-	 * was already closed
-	 */
-	if (!sock->sock->sk) {
-		DEBUG_NET_WARN_ON_ONCE(1);
-		return;
-	}
-
 	/* Drop the reference while holding the sock lock to avoid
 	 * concurrent ovpn_socket_new call to mess up with a partially
 	 * detached socket.
@@ -90,22 +82,24 @@ void ovpn_socket_release(struct ovpn_peer *peer)
 	 * Holding the lock ensures that a socket with refcnt 0 is fully
 	 * detached before it can be picked by a concurrent reader.
 	 */
-	lock_sock(sock->sock->sk);
+	lock_sock(sock->sk);
 	released = ovpn_socket_put(peer, sock);
-	release_sock(sock->sock->sk);
+	release_sock(sock->sk);
 
 	/* align all readers with sk_user_data being NULL */
 	synchronize_rcu();
 
 	/* following cleanup should happen with lock released */
 	if (released) {
-		if (sock->sock->sk->sk_protocol == IPPROTO_UDP) {
+		if (sock->sk->sk_protocol == IPPROTO_UDP) {
 			netdev_put(sock->ovpn->dev, &sock->dev_tracker);
-		} else if (sock->sock->sk->sk_protocol == IPPROTO_TCP) {
+		} else if (sock->sk->sk_protocol == IPPROTO_TCP) {
 			/* wait for TCP jobs to terminate */
 			ovpn_tcp_socket_wait_finish(sock);
 			ovpn_peer_put(sock->peer);
 		}
+		/* drop reference acquired in ovpn_socket_new() */
+		sock_put(sock->sk);
 		/* we can call plain kfree() because we already waited one RCU
 		 * period due to synchronize_rcu()
 		 */
@@ -118,12 +112,14 @@ static bool ovpn_socket_hold(struct ovpn_socket *sock)
 	return kref_get_unless_zero(&sock->refcount);
 }
 
-static int ovpn_socket_attach(struct ovpn_socket *sock, struct ovpn_peer *peer)
+static int ovpn_socket_attach(struct ovpn_socket *ovpn_sock,
+			      struct socket *sock,
+			      struct ovpn_peer *peer)
 {
-	if (sock->sock->sk->sk_protocol == IPPROTO_UDP)
-		return ovpn_udp_socket_attach(sock, peer->ovpn);
-	else if (sock->sock->sk->sk_protocol == IPPROTO_TCP)
-		return ovpn_tcp_socket_attach(sock, peer);
+	if (sock->sk->sk_protocol == IPPROTO_UDP)
+		return ovpn_udp_socket_attach(ovpn_sock, sock, peer->ovpn);
+	else if (sock->sk->sk_protocol == IPPROTO_TCP)
+		return ovpn_tcp_socket_attach(ovpn_sock, peer);
 
 	return -EOPNOTSUPP;
 }
@@ -138,14 +134,15 @@ static int ovpn_socket_attach(struct ovpn_socket *sock, struct ovpn_peer *peer)
 struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 {
 	struct ovpn_socket *ovpn_sock;
+	struct sock *sk = sock->sk;
 	int ret;
 
-	lock_sock(sock->sk);
+	lock_sock(sk);
 
 	/* a TCP socket can only be owned by a single peer, therefore there
 	 * can't be any other user
 	 */
-	if (sock->sk->sk_protocol == IPPROTO_TCP && sock->sk->sk_user_data) {
+	if (sk->sk_protocol == IPPROTO_TCP && sk->sk_user_data) {
 		ovpn_sock = ERR_PTR(-EBUSY);
 		goto sock_release;
 	}
@@ -153,8 +150,8 @@ 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 (sock->sk->sk_protocol == IPPROTO_UDP) {
-		u8 type = READ_ONCE(udp_sk(sock->sk)->encap_type);
+	if (sk->sk_protocol == IPPROTO_UDP) {
+		u8 type = READ_ONCE(udp_sk(sk)->encap_type);
 
 		/* socket owned by other encapsulation module */
 		if (type && type != UDP_ENCAP_OVPNINUDP) {
@@ -163,7 +160,7 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 		}
 
 		rcu_read_lock();
-		ovpn_sock = rcu_dereference_sk_user_data(sock->sk);
+		ovpn_sock = rcu_dereference_sk_user_data(sk);
 		if (ovpn_sock) {
 			/* socket owned by another ovpn instance, we can't use it */
 			if (ovpn_sock->ovpn != peer->ovpn) {
@@ -192,19 +189,30 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 		rcu_read_unlock();
 	}
 
+	/* increase sk refcounter as we'll store a reference in
+	 * ovpn_socket.
+	 * ovpn_socket_release() will decrement the refcounter.
+	 */
+	if (!refcount_inc_not_zero(&sk->sk_refcnt)) {
+		ovpn_sock = ERR_PTR(-ENOTSOCK);
+		goto sock_release;
+	}
+
 	/* socket is not owned: attach to this ovpn instance */
 
 	ovpn_sock = kzalloc(sizeof(*ovpn_sock), GFP_KERNEL);
 	if (!ovpn_sock) {
+		sock_put(sk);
 		ovpn_sock = ERR_PTR(-ENOMEM);
 		goto sock_release;
 	}
 
-	ovpn_sock->sock = sock;
+	ovpn_sock->sk = sk;
 	kref_init(&ovpn_sock->refcount);
 
-	ret = ovpn_socket_attach(ovpn_sock, peer);
+	ret = ovpn_socket_attach(ovpn_sock, sock, peer);
 	if (ret < 0) {
+		sock_put(sk);
 		kfree(ovpn_sock);
 		ovpn_sock = ERR_PTR(ret);
 		goto sock_release;
@@ -213,11 +221,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 (sock->sk->sk_protocol == IPPROTO_TCP) {
+	if (sk->sk_protocol == IPPROTO_TCP) {
 		INIT_WORK(&ovpn_sock->tcp_tx_work, ovpn_tcp_tx_work);
 		ovpn_sock->peer = peer;
 		ovpn_peer_hold(peer);
-	} else if (sock->sk->sk_protocol == IPPROTO_UDP) {
+	} else if (sk->sk_protocol == IPPROTO_UDP) {
 		/* in UDP we only link the ovpn instance since the socket is
 		 * shared among multiple peers
 		 */
@@ -226,8 +234,8 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 			    GFP_KERNEL);
 	}
 
-	rcu_assign_sk_user_data(sock->sk, ovpn_sock);
+	rcu_assign_sk_user_data(sk, ovpn_sock);
 sock_release:
-	release_sock(sock->sk);
+	release_sock(sk);
 	return ovpn_sock;
 }
diff --git a/drivers/net/ovpn/socket.h b/drivers/net/ovpn/socket.h
index 00d856b1a5d8..4afcec71040d 100644
--- a/drivers/net/ovpn/socket.h
+++ b/drivers/net/ovpn/socket.h
@@ -22,7 +22,7 @@ struct ovpn_peer;
  * @ovpn: ovpn instance owning this socket (UDP only)
  * @dev_tracker: reference tracker for associated dev (UDP only)
  * @peer: unique peer transmitting over this socket (TCP only)
- * @sock: the low level sock object
+ * @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)
@@ -36,7 +36,7 @@ struct ovpn_socket {
 		struct ovpn_peer *peer;
 	};
 
-	struct socket *sock;
+	struct sock *sk;
 	struct kref refcount;
 	struct work_struct work;
 	struct work_struct tcp_tx_work;
diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 7c42d84987ad..7e79aad0b043 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -186,18 +186,18 @@ static int ovpn_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 void ovpn_tcp_socket_detach(struct ovpn_socket *ovpn_sock)
 {
 	struct ovpn_peer *peer = ovpn_sock->peer;
-	struct socket *sock = ovpn_sock->sock;
+	struct sock *sk = ovpn_sock->sk;
 
 	strp_stop(&peer->tcp.strp);
 	skb_queue_purge(&peer->tcp.user_queue);
 
 	/* restore CBs that were saved in ovpn_sock_set_tcp_cb() */
-	sock->sk->sk_data_ready = peer->tcp.sk_cb.sk_data_ready;
-	sock->sk->sk_write_space = peer->tcp.sk_cb.sk_write_space;
-	sock->sk->sk_prot = peer->tcp.sk_cb.prot;
-	sock->sk->sk_socket->ops = peer->tcp.sk_cb.ops;
+	sk->sk_data_ready = peer->tcp.sk_cb.sk_data_ready;
+	sk->sk_write_space = peer->tcp.sk_cb.sk_write_space;
+	sk->sk_prot = peer->tcp.sk_cb.prot;
+	sk->sk_socket->ops = peer->tcp.sk_cb.ops;
 
-	rcu_assign_sk_user_data(sock->sk, NULL);
+	rcu_assign_sk_user_data(sk, NULL);
 }
 
 void ovpn_tcp_socket_wait_finish(struct ovpn_socket *sock)
@@ -283,10 +283,10 @@ void ovpn_tcp_tx_work(struct work_struct *work)
 
 	sock = container_of(work, struct ovpn_socket, tcp_tx_work);
 
-	lock_sock(sock->sock->sk);
+	lock_sock(sock->sk);
 	if (sock->peer)
-		ovpn_tcp_send_sock(sock->peer, sock->sock->sk);
-	release_sock(sock->sock->sk);
+		ovpn_tcp_send_sock(sock->peer, sock->sk);
+	release_sock(sock->sk);
 }
 
 static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
@@ -307,15 +307,15 @@ static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
 	ovpn_tcp_send_sock(peer, sk);
 }
 
-void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct socket *sock,
+void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 		       struct sk_buff *skb)
 {
 	u16 len = skb->len;
 
 	*(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);
 
-	spin_lock_nested(&sock->sk->sk_lock.slock, OVPN_TCP_DEPTH_NESTING);
-	if (sock_owned_by_user(sock->sk)) {
+	spin_lock_nested(&sk->sk_lock.slock, OVPN_TCP_DEPTH_NESTING);
+	if (sock_owned_by_user(sk)) {
 		if (skb_queue_len(&peer->tcp.out_queue) >=
 		    READ_ONCE(net_hotdata.max_backlog)) {
 			dev_dstats_tx_dropped(peer->ovpn->dev);
@@ -324,10 +324,10 @@ void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct socket *sock,
 		}
 		__skb_queue_tail(&peer->tcp.out_queue, skb);
 	} else {
-		ovpn_tcp_send_sock_skb(peer, sock->sk, skb);
+		ovpn_tcp_send_sock_skb(peer, sk, skb);
 	}
 unlock:
-	spin_unlock(&sock->sk->sk_lock.slock);
+	spin_unlock(&sk->sk_lock.slock);
 }
 
 static void ovpn_tcp_release(struct sock *sk)
@@ -474,7 +474,6 @@ static void ovpn_tcp_peer_del_work(struct work_struct *work)
 int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 			   struct ovpn_peer *peer)
 {
-	struct socket *sock = ovpn_sock->sock;
 	struct strp_callbacks cb = {
 		.rcv_msg = ovpn_tcp_rcv,
 		.parse_msg = ovpn_tcp_parse,
@@ -482,20 +481,20 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 	int ret;
 
 	/* make sure no pre-existing encapsulation handler exists */
-	if (sock->sk->sk_user_data)
+	if (ovpn_sock->sk->sk_user_data)
 		return -EBUSY;
 
 	/* only a fully connected socket is expected. Connection should be
 	 * handled in userspace
 	 */
-	if (sock->sk->sk_state != TCP_ESTABLISHED) {
+	if (ovpn_sock->sk->sk_state != TCP_ESTABLISHED) {
 		net_err_ratelimited("%s: provided TCP socket is not in ESTABLISHED state: %d\n",
 				    netdev_name(peer->ovpn->dev),
-				    sock->sk->sk_state);
+				    ovpn_sock->sk->sk_state);
 		return -EINVAL;
 	}
 
-	ret = strp_init(&peer->tcp.strp, sock->sk, &cb);
+	ret = strp_init(&peer->tcp.strp, ovpn_sock->sk, &cb);
 	if (ret < 0) {
 		DEBUG_NET_WARN_ON_ONCE(1);
 		return ret;
@@ -503,31 +502,31 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 
 	INIT_WORK(&peer->tcp.defer_del_work, ovpn_tcp_peer_del_work);
 
-	__sk_dst_reset(sock->sk);
+	__sk_dst_reset(ovpn_sock->sk);
 	skb_queue_head_init(&peer->tcp.user_queue);
 	skb_queue_head_init(&peer->tcp.out_queue);
 
 	/* save current CBs so that they can be restored upon socket release */
-	peer->tcp.sk_cb.sk_data_ready = sock->sk->sk_data_ready;
-	peer->tcp.sk_cb.sk_write_space = sock->sk->sk_write_space;
-	peer->tcp.sk_cb.prot = sock->sk->sk_prot;
-	peer->tcp.sk_cb.ops = sock->sk->sk_socket->ops;
+	peer->tcp.sk_cb.sk_data_ready = ovpn_sock->sk->sk_data_ready;
+	peer->tcp.sk_cb.sk_write_space = ovpn_sock->sk->sk_write_space;
+	peer->tcp.sk_cb.prot = ovpn_sock->sk->sk_prot;
+	peer->tcp.sk_cb.ops = ovpn_sock->sk->sk_socket->ops;
 
 	/* assign our static CBs and prot/ops */
-	sock->sk->sk_data_ready = ovpn_tcp_data_ready;
-	sock->sk->sk_write_space = ovpn_tcp_write_space;
+	ovpn_sock->sk->sk_data_ready = ovpn_tcp_data_ready;
+	ovpn_sock->sk->sk_write_space = ovpn_tcp_write_space;
 
-	if (sock->sk->sk_family == AF_INET) {
-		sock->sk->sk_prot = &ovpn_tcp_prot;
-		sock->sk->sk_socket->ops = &ovpn_tcp_ops;
+	if (ovpn_sock->sk->sk_family == AF_INET) {
+		ovpn_sock->sk->sk_prot = &ovpn_tcp_prot;
+		ovpn_sock->sk->sk_socket->ops = &ovpn_tcp_ops;
 	} else {
-		sock->sk->sk_prot = &ovpn_tcp6_prot;
-		sock->sk->sk_socket->ops = &ovpn_tcp6_ops;
+		ovpn_sock->sk->sk_prot = &ovpn_tcp6_prot;
+		ovpn_sock->sk->sk_socket->ops = &ovpn_tcp6_ops;
 	}
 
 	/* avoid using task_frag */
-	sock->sk->sk_allocation = GFP_ATOMIC;
-	sock->sk->sk_use_task_frag = false;
+	ovpn_sock->sk->sk_allocation = GFP_ATOMIC;
+	ovpn_sock->sk->sk_use_task_frag = false;
 
 	/* enqueue the RX worker */
 	strp_check_rcv(&peer->tcp.strp);
diff --git a/drivers/net/ovpn/tcp.h b/drivers/net/ovpn/tcp.h
index 10aefa834cf3..a3aa3570ae5e 100644
--- a/drivers/net/ovpn/tcp.h
+++ b/drivers/net/ovpn/tcp.h
@@ -30,7 +30,8 @@ void ovpn_tcp_socket_wait_finish(struct ovpn_socket *sock);
  * 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 socket *sock, struct sk_buff *skb);
+void 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_ */
diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index c8a81c4d6489..b4fbebad8f45 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -43,7 +43,7 @@ static struct ovpn_socket *ovpn_socket_from_udp_sock(struct sock *sk)
 		return NULL;
 
 	/* make sure that sk matches our stored transport socket */
-	if (unlikely(!ovpn_sock->sock || sk != ovpn_sock->sock->sk))
+	if (unlikely(!ovpn_sock->sk || sk != ovpn_sock->sk))
 		return NULL;
 
 	return ovpn_sock;
@@ -335,32 +335,22 @@ static int ovpn_udp_output(struct ovpn_peer *peer, struct dst_cache *cache,
 /**
  * ovpn_udp_send_skb - prepare skb and send it over via UDP
  * @peer: the destination peer
- * @sock: the RCU protected peer socket
+ * @sk: peer socket
  * @skb: the packet to send
  */
-void ovpn_udp_send_skb(struct ovpn_peer *peer, struct socket *sock,
+void ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 		       struct sk_buff *skb)
 {
-	int ret = -1;
+	int ret;
 
 	skb->dev = peer->ovpn->dev;
 	/* no checksum performed at this layer */
 	skb->ip_summed = CHECKSUM_NONE;
 
-	/* get socket info */
-	if (unlikely(!sock)) {
-		net_warn_ratelimited("%s: no sock for remote peer %u\n",
-				     netdev_name(peer->ovpn->dev), peer->id);
-		goto out;
-	}
-
 	/* crypto layer -> transport (UDP) */
-	ret = ovpn_udp_output(peer, &peer->dst_cache, sock->sk, skb);
-out:
-	if (unlikely(ret < 0)) {
+	ret = ovpn_udp_output(peer, &peer->dst_cache, sk, skb);
+	if (unlikely(ret < 0))
 		kfree_skb(skb);
-		return;
-	}
 }
 
 static void ovpn_udp_encap_destroy(struct sock *sk)
@@ -383,6 +373,7 @@ static void ovpn_udp_encap_destroy(struct sock *sk)
 /**
  * ovpn_udp_socket_attach - set udp-tunnel CBs on socket and link it to ovpn
  * @ovpn_sock: socket to configure
+ * @sock: the socket container to be passed to setup_udp_tunnel_sock()
  * @ovpn: the openvp instance to link
  *
  * After invoking this function, the sock will be controlled by ovpn so that
@@ -390,7 +381,7 @@ static void ovpn_udp_encap_destroy(struct sock *sk)
  *
  * Return: 0 on success or a negative error code otherwise
  */
-int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
+int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock, struct socket *sock,
 			   struct ovpn_priv *ovpn)
 {
 	struct udp_tunnel_sock_cfg cfg = {
@@ -398,17 +389,16 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
 		.encap_rcv = ovpn_udp_encap_recv,
 		.encap_destroy = ovpn_udp_encap_destroy,
 	};
-	struct socket *sock = ovpn_sock->sock;
 	struct ovpn_socket *old_data;
 	int ret;
 
 	/* make sure no pre-existing encapsulation handler exists */
 	rcu_read_lock();
-	old_data = rcu_dereference_sk_user_data(sock->sk);
+	old_data = rcu_dereference_sk_user_data(ovpn_sock->sk);
 	if (!old_data) {
 		/* socket is currently unused - we can take it */
 		rcu_read_unlock();
-		setup_udp_tunnel_sock(sock_net(sock->sk), sock, &cfg);
+		setup_udp_tunnel_sock(sock_net(ovpn_sock->sk), sock, &cfg);
 		return 0;
 	}
 
@@ -421,7 +411,7 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
 	 * Unlikely TCP, a single UDP socket can be used to talk to many remote
 	 * hosts and therefore openvpn instantiates one only for all its peers
 	 */
-	if ((READ_ONCE(udp_sk(sock->sk)->encap_type) == UDP_ENCAP_OVPNINUDP) &&
+	if ((READ_ONCE(udp_sk(ovpn_sock->sk)->encap_type) == UDP_ENCAP_OVPNINUDP) &&
 	    old_data->ovpn == ovpn) {
 		netdev_dbg(ovpn->dev,
 			   "provided socket already owned by this interface\n");
@@ -442,5 +432,5 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
  */
 void ovpn_udp_socket_detach(struct ovpn_socket *ovpn_sock)
 {
-	cleanup_udp_tunnel_sock(ovpn_sock->sock->sk);
+	cleanup_udp_tunnel_sock(ovpn_sock->sk);
 }
diff --git a/drivers/net/ovpn/udp.h b/drivers/net/ovpn/udp.h
index 9994eb6e0428..fe26fbe25c5a 100644
--- a/drivers/net/ovpn/udp.h
+++ b/drivers/net/ovpn/udp.h
@@ -15,11 +15,11 @@ struct ovpn_peer;
 struct ovpn_priv;
 struct socket;
 
-int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
+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 socket *sock,
+void ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 		       struct sk_buff *skb);
 
 #endif /* _NET_OVPN_UDP_H_ */
-- 
2.49.0

[PATCH net-next 3/4] selftest/net/ovpn: fix TCP socket creation

[Antonio Quartulli]

TCP sockets cannot be created with AF_UNSPEC, but
one among the supported family must be used.

Since commit 944f8b6abab6 ("selftest/net/ovpn: extend
coverage with more test cases") the default address
family for all tests was changed from AF_INET to AF_UNSPEC,
thus breaking all TCP cases.

Restore AF_INET as default address family for TCP listeners.

Fixes: 944f8b6abab6 ("selftest/net/ovpn: extend coverage with more test cases")
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 tools/testing/selftests/net/ovpn/ovpn-cli.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c
index de9c26f98b2e..9201f2905f2c 100644
--- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
+++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
@@ -2166,6 +2166,7 @@ static int ovpn_parse_cmd_args(struct ovpn_ctx *ovpn, int argc, char *argv[])
 
 		ovpn->peers_file = argv[4];
 
+		ovpn->sa_family = AF_INET;
 		if (argc > 5 && !strcmp(argv[5], "ipv6"))
 			ovpn->sa_family = AF_INET6;
 		break;
-- 
2.49.0

[PATCH net-next 4/4] selftest/net/ovpn: fix missing file

[Antonio Quartulli]

test-large-mtu.sh is referenced by the Makefile
but does not exist.

Add it along the other scripts.

Fixes: 944f8b6abab6 ("selftest/net/ovpn: extend coverage with more test cases")
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 tools/testing/selftests/net/ovpn/test-large-mtu.sh | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100755 tools/testing/selftests/net/ovpn/test-large-mtu.sh

diff --git a/tools/testing/selftests/net/ovpn/test-large-mtu.sh b/tools/testing/selftests/net/ovpn/test-large-mtu.sh
new file mode 100755
index 000000000000..ce2a2cb64f72
--- /dev/null
+++ b/tools/testing/selftests/net/ovpn/test-large-mtu.sh
@@ -0,0 +1,9 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2025 OpenVPN, Inc.
+#
+#  Author:	Antonio Quartulli <antonio@openvpn.net>
+
+MTU="1500"
+
+source test.sh
-- 
2.49.0

Re: [PATCH net-next 1/4] ovpn: properly deconfigure UDP-tunnel

[Paolo Abeni]

On 5/27/25 3:46 PM, Antonio Quartulli wrote:
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index dde52b8050b8..9ffc4e0b1644 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2900,7 +2900,7 @@ void udp_destroy_sock(struct sock *sk)
>  			if (encap_destroy)
>  				encap_destroy(sk);
>  		}
> -		if (udp_test_bit(ENCAP_ENABLED, sk)) {
> +		if (udp_test_and_clear_bit(ENCAP_ENABLED, sk)) {

Nothing should use 'sk' after udp_destroy_sock(), no need to clear the
bit (same in ipv6 code)

>  			static_branch_dec(&udp_encap_needed_key);
>  			udp_tunnel_cleanup_gro(sk);
>  		}
> diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
> index 2326548997d3..624b6afcf812 100644
> --- a/net/ipv4/udp_tunnel_core.c
> +++ b/net/ipv4/udp_tunnel_core.c
> @@ -98,6 +98,28 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
>  }
>  EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
>  
> +void cleanup_udp_tunnel_sock(struct sock *sk)
> +{
> +	/* Re-enable multicast loopback */
> +	inet_set_bit(MC_LOOP, sk);
> +
> +	/* Disable CHECKSUM_UNNECESSARY to CHECKSUM_COMPLETE conversion */
> +	inet_dec_convert_csum(sk);
> +
> +	udp_sk(sk)->encap_type = 0;
> +	udp_sk(sk)->encap_rcv = NULL;
> +	udp_sk(sk)->encap_err_rcv = NULL;
> +	udp_sk(sk)->encap_err_lookup = NULL;
> +	udp_sk(sk)->encap_destroy = NULL;
> +	udp_sk(sk)->gro_receive = NULL;
> +	udp_sk(sk)->gro_complete = NULL;
> +
> +	rcu_assign_sk_user_data(sk, NULL);
> +	udp_tunnel_encap_disable(sk);
> +	udp_tunnel_cleanup_gro(sk);
> +}
> +EXPORT_SYMBOL_GPL(cleanup_udp_tunnel_sock);

IMHO the above code should not land into a generic tunnel helper, as the
tunnel scope always correspond to the sk scope - except for openvpn.

Also IMHO no need to do udp_tunnel_encap_disable(), it will called as
needed at sk close() time. Yep, that means that the stack will have
transient additional overhead until the sock is not destroyed, but IMHO
it's not worthy the extra complexity (export symbol, additional stub,
more files touched...)

/P

Re: [PATCH net-next 1/4] ovpn: properly deconfigure UDP-tunnel

[Antonio Quartulli]

Hi Paolo,

On 29/05/2025 11:25, Paolo Abeni wrote:
> On 5/27/25 3:46 PM, Antonio Quartulli wrote:
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index dde52b8050b8..9ffc4e0b1644 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -2900,7 +2900,7 @@ void udp_destroy_sock(struct sock *sk)
>>   			if (encap_destroy)
>>   				encap_destroy(sk);
>>   		}
>> -		if (udp_test_bit(ENCAP_ENABLED, sk)) {
>> +		if (udp_test_and_clear_bit(ENCAP_ENABLED, sk)) {
> 
> Nothing should use 'sk' after udp_destroy_sock(), no need to clear the
> bit (same in ipv6 code)

Ok, then I guess that in ovpn I need to check if SOCK_DEAD is set and, 
if so, just bail out.

> 
>>   			static_branch_dec(&udp_encap_needed_key);
>>   			udp_tunnel_cleanup_gro(sk);
>>   		}
>> diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
>> index 2326548997d3..624b6afcf812 100644
>> --- a/net/ipv4/udp_tunnel_core.c
>> +++ b/net/ipv4/udp_tunnel_core.c
>> @@ -98,6 +98,28 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
>>   }
>>   EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
>>   
>> +void cleanup_udp_tunnel_sock(struct sock *sk)
>> +{
>> +	/* Re-enable multicast loopback */
>> +	inet_set_bit(MC_LOOP, sk);
>> +
>> +	/* Disable CHECKSUM_UNNECESSARY to CHECKSUM_COMPLETE conversion */
>> +	inet_dec_convert_csum(sk);
>> +
>> +	udp_sk(sk)->encap_type = 0;
>> +	udp_sk(sk)->encap_rcv = NULL;
>> +	udp_sk(sk)->encap_err_rcv = NULL;
>> +	udp_sk(sk)->encap_err_lookup = NULL;
>> +	udp_sk(sk)->encap_destroy = NULL;
>> +	udp_sk(sk)->gro_receive = NULL;
>> +	udp_sk(sk)->gro_complete = NULL;
>> +
>> +	rcu_assign_sk_user_data(sk, NULL);
>> +	udp_tunnel_encap_disable(sk);
>> +	udp_tunnel_cleanup_gro(sk);
>> +}
>> +EXPORT_SYMBOL_GPL(cleanup_udp_tunnel_sock);
> 
> IMHO the above code should not land into a generic tunnel helper, as the
> tunnel scope always correspond to the sk scope - except for openvpn.
> 

Ok.
I thought udp_tunnel was the right place because this helper is an 
explicit counterpart for setup_udp_tunnel_sock().

But since there are no other users, I can move this code inside ovpn.

> Also IMHO no need to do udp_tunnel_encap_disable(), it will called as
> needed at sk close() time. Yep, that means that the stack will have
> transient additional overhead until the sock is not destroyed, but IMHO
> it's not worthy the extra complexity (export symbol, additional stub,
> more files touched...)

That's true: removing that part will greatly simplify the patch.

If we agree that we can live with the transient overhead, then ok, I 
will remove the udp_tunnel_encap_disable() part and simplify this commit.


Thanks for the feedback, Paolo!


Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net-next 2/4] ovpn: ensure sk is still valid during cleanup

[Paolo Abeni]

On 5/27/25 3:46 PM, Antonio Quartulli wrote:
> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
> index bea03913bfb1..1dd7e763c168 100644
> --- a/drivers/net/ovpn/netlink.c
> +++ b/drivers/net/ovpn/netlink.c
> @@ -423,9 +423,14 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>  	ovpn_sock = ovpn_socket_new(sock, peer);
>  	/* at this point we unconditionally drop the reference to the socket:
>  	 * - in case of error, the socket has to be dropped
> -	 * - if case of success, the socket is configured and let
> +	 * - if case of success, the socket is configured and we let
>  	 *   userspace own the reference, so that the latter can
> -	 *   trigger the final close()
> +	 *   trigger the final close().
> +	 *
> +	 * NOTE: at this point ovpn_socket_new() has acquired a reference
> +	 * to sock->sk. That's needed especially to avoid race conditions
> +	 * during cleanup, where sock may still be alive, but sock->sk may be
> +	 * getting released concurrently.

This comment duplicate some wording from commit message contents and
don't add much value IMHO. It could be dropped.

> @@ -192,19 +189,30 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
>  		rcu_read_unlock();
>  	}
>  
> +	/* increase sk refcounter as we'll store a reference in
> +	 * ovpn_socket.
> +	 * ovpn_socket_release() will decrement the refcounter.
> +	 */
> +	if (!refcount_inc_not_zero(&sk->sk_refcnt)) {

How could sk_refcnt be zero here? likely just sock_hold() is sufficient.
Also I think the reference could be acquired a little later, avoiding at
least the following chunk.

Also IMHO the comment is not very clear. I think it should state
explicitly which entity is retaining the reference (AFAICS the peer hash
table).

/P

Re: [PATCH net-next 2/4] ovpn: ensure sk is still valid during cleanup

[Antonio Quartulli]

On 29/05/2025 11:44, Paolo Abeni wrote:
> On 5/27/25 3:46 PM, Antonio Quartulli wrote:
>> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
>> index bea03913bfb1..1dd7e763c168 100644
>> --- a/drivers/net/ovpn/netlink.c
>> +++ b/drivers/net/ovpn/netlink.c
>> @@ -423,9 +423,14 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>>   	ovpn_sock = ovpn_socket_new(sock, peer);
>>   	/* at this point we unconditionally drop the reference to the socket:
>>   	 * - in case of error, the socket has to be dropped
>> -	 * - if case of success, the socket is configured and let
>> +	 * - if case of success, the socket is configured and we let
>>   	 *   userspace own the reference, so that the latter can
>> -	 *   trigger the final close()
>> +	 *   trigger the final close().
>> +	 *
>> +	 * NOTE: at this point ovpn_socket_new() has acquired a reference
>> +	 * to sock->sk. That's needed especially to avoid race conditions
>> +	 * during cleanup, where sock may still be alive, but sock->sk may be
>> +	 * getting released concurrently.
> 
> This comment duplicate some wording from commit message contents and
> don't add much value IMHO. It could be dropped.

ACK

> 
>> @@ -192,19 +189,30 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
>>   		rcu_read_unlock();
>>   	}
>>   
>> +	/* increase sk refcounter as we'll store a reference in
>> +	 * ovpn_socket.
>> +	 * ovpn_socket_release() will decrement the refcounter.
>> +	 */
>> +	if (!refcount_inc_not_zero(&sk->sk_refcnt)) {
> 
> How could sk_refcnt be zero here? likely just sock_hold() is sufficient.
> Also I think the reference could be acquired a little later, avoiding at
> least the following chunk.

I just wanted to be safe.
But as you point out I don't see any reason why sk_refcnt would be 0 at 
this point in time.

If we don't have to check the return value, I can definitely postpone 
the sock_hold() a bit more.

> 
> Also IMHO the comment is not very clear. I think it should state
> explicitly which entity is retaining the reference (AFAICS the peer hash
> table).

Ok, I will elaborate a bit more.
Essentially: sk -> ovpn_socket -> ovpn_peer -> peer hash


Thanks a lot!
Regards,

> 
> /P
> 

-- 
Antonio Quartulli
OpenVPN Inc.