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")
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>
---
v2:
- handle nesting across different transport sockets too (two peers routed
through each other, cross-CPU ABBA, lockdep splat), as pointed out by Sashiko
- defer such transmissions to the TX worker instead of dropping self-looped
packets in ovpn_net_xmit()
drivers/net/ovpn/io.c | 2 +-
drivers/net/ovpn/peer.h | 2 +-
drivers/net/ovpn/tcp.c | 103 +++++++++++++++++++++++++++++-----------
drivers/net/ovpn/tcp.h | 2 +-
4 files changed, 79 insertions(+), 30 deletions(-)
@@ -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 */
@@ -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)
@@ -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,82 @@ 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 {
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 +444,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);
@@ -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);