diff mbox series

[Openvpn-devel,net,v3] ovpn: avoid nesting TCP transport socket locks

Message ID 20261031225640.1436694-1-a@unstable.cc
State New
Headers
Series [Openvpn-devel,net,v3] ovpn: avoid nesting TCP transport socket locks |

Commit Message

Antonio Quartulli Oct. 31, 2026, 10:56 p.m. UTC 
  From: Antonio Quartulli <antonio@openvpn.net>

ovpn_tcp_send_skb() and ovpn_tcp_release() transmit while holding the
transport socket spinlock. If the outer packet is routed into an ovpn
interface, ovpn_tcp_send_skb() takes the spinlock of the selected peer's
socket while the first one is still held. This deadlocks when it is the
same socket or when two peers are routed through each other.

Defer transmission to the TX worker when the skb comes from an ovpn TCP
transport socket.

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Cc: stable@vger.kernel.org
Reported-by: Bernhard Schmidt <berni@birkenwald.de>
Closes: https://lore.kernel.org/openvpn-devel/auJj5Ld6uMjPb2eS@fliwatuet.svr02.mucip.net/
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---

v3:
- queue new packets behind already deferred ones to preserve ordering (Sashiko)
v2: https://lore.kernel.org/all/20261029144241.1671514-1-a@unstable.cc/

 drivers/net/ovpn/io.c   |   2 +-
 drivers/net/ovpn/peer.h |   2 +-
 drivers/net/ovpn/tcp.c  | 109 ++++++++++++++++++++++++++++++----------
 drivers/net/ovpn/tcp.h  |   2 +-
 4 files changed, 85 insertions(+), 30 deletions(-)
diff mbox series

Patch

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index f14eb4e6c46a..3282c3b0bf58 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -284,7 +284,7 @@  void ovpn_encrypt_post(void *data, int ret)
 		ovpn_udp_send_skb(peer, sock->sk, skb);
 		break;
 	case IPPROTO_TCP:
-		ovpn_tcp_send_skb(peer, sock->sk, skb);
+		ovpn_tcp_send_skb(peer, sock, skb);
 		break;
 	default:
 		/* no transport configured yet */
diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index 1879bfb76992..8256c628f769 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h
@@ -45,7 +45,7 @@  struct ovpn_route_key {
  * @tcp: keeps track of TCP specific state
  * @tcp.strp: stream parser context (TCP only)
  * @tcp.user_queue: received packets that have to go to userspace (TCP only)
- * @tcp.out_queue: packets on hold while socket is taken by user (TCP only)
+ * @tcp.out_queue: packets deferred for later transmission (TCP only)
  * @tcp.tx_in_progress: true if TX is already ongoing (TCP only)
  * @tcp.out_msg.skb: packet scheduled for sending (TCP only)
  * @tcp.out_msg.offset: offset where next send should start (TCP only)
diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 8fe8a8e750a4..815d5523c43b 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -313,6 +313,35 @@  static void ovpn_tcp_send_sock(struct ovpn_peer *peer, struct sock *sk)
 	peer->tcp.tx_in_progress = false;
 }
 
+static void 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;
+	}
+
+	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);
+}
+
+/* send queued packets, stop if one remains pending */
+static void ovpn_tcp_flush_queue(struct ovpn_peer *peer, struct sock *sk)
+{
+	unsigned int n = skb_queue_len_lockless(&peer->tcp.out_queue);
+	struct sk_buff *skb;
+
+	while (n-- && !peer->tcp.out_msg.skb &&
+	       (skb = skb_dequeue(&peer->tcp.out_queue)))
+		ovpn_tcp_send_sock_skb(peer, sk, skb);
+}
+
 void ovpn_tcp_tx_work(struct work_struct *work)
 {
 	struct ovpn_socket *sock;
@@ -320,58 +349,88 @@  void ovpn_tcp_tx_work(struct work_struct *work)
 	sock = container_of(work, struct ovpn_socket, tcp_tx_work);
 
 	lock_sock(sock->sk);
-	if (sock->peer)
+	if (sock->peer) {
 		ovpn_tcp_send_sock(sock->peer, sock->sk);
+		ovpn_tcp_flush_queue(sock->peer, sock->sk);
+	}
 	release_sock(sock->sk);
 }
 
-static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sock *sk,
-				   struct sk_buff *skb)
+static bool ovpn_tcp_enqueue(struct ovpn_peer *peer, struct sk_buff *skb)
 {
-	if (peer->tcp.out_msg.skb)
-		ovpn_tcp_send_sock(peer, sk);
+	struct sk_buff_head *queue = &peer->tcp.out_queue;
+	unsigned long flags;
+	bool ret = false;
+
+	spin_lock_irqsave(&queue->lock, flags);
+	if (skb_queue_len(queue) < READ_ONCE(net_hotdata.max_backlog)) {
+		__skb_queue_tail(queue, skb);
+		ret = true;
+	}
+	spin_unlock_irqrestore(&queue->lock, flags);
 
-	if (peer->tcp.out_msg.skb) {
+	if (!ret) {
 		ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
 		kfree_skb(skb);
-		return;
 	}
 
-	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 ret;
+}
+
+/**
+ * ovpn_tcp_skb_from_transport - check if skb comes from an ovpn TCP socket
+ * @skb: the packet to check
+ *
+ * Return: true if @skb was generated by an ovpn TCP transport socket
+ */
+static bool ovpn_tcp_skb_from_transport(const struct sk_buff *skb)
+{
+	const struct proto *prot;
+
+	if (!skb->sk)
+		return false;
+
+	prot = READ_ONCE(skb->sk->sk_prot);
+	return prot == &ovpn_tcp_prot || prot == &ovpn_tcp6_prot;
 }
 
-void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sock *sk,
+void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct ovpn_socket *sock,
 		       struct sk_buff *skb)
 {
+	struct sock *sk = sock->sk;
 	u16 len = skb->len;
 
 	*(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);
 
+	/* the spinlock of the originating transport socket may be held:
+	 * defer to the worker to avoid nesting socket locks
+	 */
+	if (ovpn_tcp_skb_from_transport(skb)) {
+		if (ovpn_tcp_enqueue(peer, skb))
+			queue_work(ovpn_wq, &sock->tcp_tx_work);
+		return;
+	}
+
 	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)) {
-			ovpn_dev_dstats_tx_dropped(peer->ovpn->dev);
-			kfree_skb(skb);
-			goto unlock;
-		}
-		__skb_queue_tail(&peer->tcp.out_queue, skb);
+		/* flushed by ovpn_tcp_release() */
+		ovpn_tcp_enqueue(peer, skb);
+	} else if (!skb_queue_empty_lockless(&peer->tcp.out_queue)) {
+		/* preserve ordering with deferred packets.
+		 * If out_msg is pending, write_space resumes TX
+		 */
+		if (ovpn_tcp_enqueue(peer, skb) && !peer->tcp.out_msg.skb)
+			queue_work(ovpn_wq, &sock->tcp_tx_work);
 	} else {
 		ovpn_tcp_send_sock_skb(peer, sk, skb);
 	}
-unlock:
 	spin_unlock(&sk->sk_lock.slock);
 }
 
 static void ovpn_tcp_release(struct sock *sk)
 {
-	struct sk_buff_head queue;
 	struct ovpn_socket *sock;
 	struct ovpn_peer *peer;
-	struct sk_buff *skb;
 
 	rcu_read_lock();
 	sock = rcu_dereference_sk_user_data(sk);
@@ -391,11 +450,7 @@  static void ovpn_tcp_release(struct sock *sk)
 	}
 	rcu_read_unlock();
 
-	__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);
+	ovpn_tcp_flush_queue(peer, sk);
 
 	peer->tcp.sk_cb.prot->release_cb(sk);
 	ovpn_peer_put(peer);
diff --git a/drivers/net/ovpn/tcp.h b/drivers/net/ovpn/tcp.h
index a3aa3570ae5e..9f2b5efcbd3a 100644
--- a/drivers/net/ovpn/tcp.h
+++ b/drivers/net/ovpn/tcp.h
@@ -30,7 +30,7 @@  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 sock *sk,
+void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct ovpn_socket *sock,
 		       struct sk_buff *skb);
 void ovpn_tcp_tx_work(struct work_struct *work);