Re: [PATCH net-next v11 12/23] ovpn: implement TCP transport

[Sabrina Dubroca <sd@queasysnail.net>]

2024-10-29, 11:47:25 +0100, Antonio Quartulli wrote:
> +static void ovpn_socket_release_work(struct work_struct *work)
> +{
> +	struct ovpn_socket *sock = container_of(work, struct ovpn_socket, work);
> +
> +	ovpn_socket_detach(sock->sock);
> +	kfree_rcu(sock, rcu);
> +}
> +
> +static void ovpn_socket_schedule_release(struct ovpn_socket *sock)
> +{
> +	INIT_WORK(&sock->work, ovpn_socket_release_work);
> +	schedule_work(&sock->work);

How does module unloading know that it has to wait for this work to
complete? Will ovpn_cleanup get stuck until some refcount gets
released by this work?


[...]
> +static void ovpn_tcp_rcv(struct strparser *strp, struct sk_buff *skb)
> +{
> +	struct ovpn_peer *peer = container_of(strp, struct ovpn_peer, tcp.strp);
> +	struct strp_msg *msg = strp_msg(skb);
> +	size_t pkt_len = msg->full_len - 2;
> +	size_t off = msg->offset + 2;
> +
> +	/* ensure skb->data points to the beginning of the openvpn packet */
> +	if (!pskb_pull(skb, off)) {
> +		net_warn_ratelimited("%s: packet too small\n",
> +				     peer->ovpn->dev->name);
> +		goto err;
> +	}
> +
> +	/* strparser does not trim the skb for us, therefore we do it now */
> +	if (pskb_trim(skb, pkt_len) != 0) {
> +		net_warn_ratelimited("%s: trimming skb failed\n",
> +				     peer->ovpn->dev->name);
> +		goto err;
> +	}
> +
> +	/* we need the first byte of data to be accessible
> +	 * to extract the opcode and the key ID later on
> +	 */
> +	if (!pskb_may_pull(skb, 1)) {
> +		net_warn_ratelimited("%s: packet too small to fetch opcode\n",
> +				     peer->ovpn->dev->name);
> +		goto err;
> +	}
> +
> +	/* DATA_V2 packets are handled in kernel, the rest goes to user space */
> +	if (likely(ovpn_opcode_from_skb(skb, 0) == OVPN_DATA_V2)) {
> +		/* hold reference to peer as required by ovpn_recv().
> +		 *
> +		 * NOTE: in this context we should already be holding a
> +		 * reference to this peer, therefore ovpn_peer_hold() is
> +		 * not expected to fail
> +		 */
> +		if (WARN_ON(!ovpn_peer_hold(peer)))
> +			goto err;
> +
> +		ovpn_recv(peer, skb);
> +	} else {
> +		/* The packet size header must be there when sending the packet
> +		 * to userspace, therefore we put it back
> +		 */
> +		skb_push(skb, 2);
> +		ovpn_tcp_to_userspace(peer, strp->sk, skb);
> +	}
> +
> +	return;
> +err:
> +	netdev_err(peer->ovpn->dev,
> +		   "cannot process incoming TCP data for peer %u\n", peer->id);

This should also be ratelimited, and maybe just combined with the
net_warn_ratelimited just before each goto.

> +	dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
> +	kfree_skb(skb);
> +	ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR);
> +}

[...]
> +void ovpn_tcp_socket_detach(struct socket *sock)
> +{
[...]
> +	/* 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;
> +	rcu_assign_sk_user_data(sock->sk, NULL);
> +
> +	rcu_read_unlock();
> +
> +	/* cancel any ongoing work. Done after removing the CBs so that these
> +	 * workers cannot be re-armed
> +	 */

I'm not sure whether a barrier is needed to prevent compiler/CPU
reordering here.

> +	cancel_work_sync(&peer->tcp.tx_work);
> +	strp_done(&peer->tcp.strp);
> +}
> +
> +static void ovpn_tcp_send_sock(struct ovpn_peer *peer)
> +{
> +	struct sk_buff *skb = peer->tcp.out_msg.skb;
> +
> +	if (!skb)
> +		return;
> +
> +	if (peer->tcp.tx_in_progress)
> +		return;
> +
> +	peer->tcp.tx_in_progress = true;

Sorry, I never answered your question about my concerns in a previous
review here.

We can reach ovpn_tcp_send_sock in two different contexts:
 - lock_sock (either from ovpn_tcp_sendmsg or ovpn_tcp_tx_work)
 - bh_lock_sock (from ovpn_tcp_send_skb, ie "data path")

These are not fully mutually exclusive. lock_sock grabs bh_lock_sock
(a spinlock) for a brief period to mark the (sleeping/mutex) lock as
taken, and then releases it.

So when bh_lock_sock is held, it's not possible to grab lock_sock. But
when lock_sock is taken, it's still possible to grab bh_lock_sock.


The buggy scenario would be:

  (data path encrypt)                       (sendmsg)
  ovpn_tcp_send_skb                         lock_sock
                                              bh_lock_sock + owned=1 + bh_unlock_sock
  bh_lock_sock
  ovpn_tcp_send_sock_skb                      ovpn_tcp_send_sock_skb
    !peer->tcp.out_msg.skb                      !peer->tcp.out_msg.skb
    peer->tcp.out_msg.skb = ...                 peer->tcp.out_msg.skb = ...
    ovpn_tcp_send_sock                          ovpn_tcp_send_sock
      !peer->tcp.tx_in_progress                   !peer->tcp.tx_in_progress
      peer->tcp.tx_in_progress = true             peer->tcp.tx_in_progress = true
      // proceed                                  // proceed


That's 2 similar races, one on out_msg.skb and one on tx_in_progress.
It's a bit unlikely (but not impossible) that we'll have 2 cpus trying
to call skb_send_sock_locked at the same time, but if they just
overwrite each other's skb/len it's already pretty bad. The end of
ovpn_tcp_send_sock might also reset peer->tcp.out_msg.* just as
ovpn_tcp_send_skb -> ovpn_tcp_send_sock_skb starts setting it up
(peer->tcp.out_msg.skb gets cleared, ovpn_tcp_send_sock_skb proceeds
and sets skb+len, then maybe len gets reset to 0 by
ovpn_tcp_send_sock).


To avoid this problem, esp_output_tcp_finish (net/ipv4/esp4.c) does:

	bh_lock_sock(sk);
	if (sock_owned_by_user(sk))
		err = espintcp_queue_out(sk, skb);
	else
		err = espintcp_push_skb(sk, skb);
	bh_unlock_sock(sk);


(espintcp_push_skb is roughly equivalent to ovpn_tcp_send_sock_skb)


> +	do {
> +		int ret = skb_send_sock_locked(peer->sock->sock->sk, skb,
> +					       peer->tcp.out_msg.offset,
> +					       peer->tcp.out_msg.len);
> +		if (unlikely(ret < 0)) {
> +			if (ret == -EAGAIN)
> +				goto out;
> +
> +			net_warn_ratelimited("%s: TCP error to peer %u: %d\n",
> +					     peer->ovpn->dev->name, peer->id,
> +					     ret);
> +
> +			/* in case of TCP error we can't recover the VPN
> +			 * stream therefore we abort the connection
> +			 */
> +			ovpn_peer_del(peer,
> +				      OVPN_DEL_PEER_REASON_TRANSPORT_ERROR);
> +			break;
> +		}
> +
> +		peer->tcp.out_msg.len -= ret;
> +		peer->tcp.out_msg.offset += ret;
> +	} while (peer->tcp.out_msg.len > 0);
> +
> +	if (!peer->tcp.out_msg.len)
> +		dev_sw_netstats_tx_add(peer->ovpn->dev, 1, skb->len);
> +
> +	kfree_skb(peer->tcp.out_msg.skb);
> +	peer->tcp.out_msg.skb = NULL;
> +	peer->tcp.out_msg.len = 0;
> +	peer->tcp.out_msg.offset = 0;
> +
> +out:
> +	peer->tcp.tx_in_progress = false;
> +}
> +
> +static void ovpn_tcp_tx_work(struct work_struct *work)
> +{
> +	struct ovpn_peer *peer;
> +
> +	peer = container_of(work, struct ovpn_peer, tcp.tx_work);
> +
> +	lock_sock(peer->sock->sock->sk);
> +	ovpn_tcp_send_sock(peer);
> +	release_sock(peer->sock->sock->sk);
> +}
> +
> +void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sk_buff *skb)
> +{
> +	if (peer->tcp.out_msg.skb)
> +		return;

That's leaking the skb? (and not counting the drop)

> +
> +	peer->tcp.out_msg.skb = skb;
> +	peer->tcp.out_msg.len = skb->len;
> +	peer->tcp.out_msg.offset = 0;
> +
> +	ovpn_tcp_send_sock(peer);
> +}
> +
> +static int ovpn_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> +{
[...]
> +	ret = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, size);
> +	if (ret) {
> +		kfree_skb(skb);
> +		net_err_ratelimited("%s: skb copy from iter failed: %d\n",
> +				    sock->peer->ovpn->dev->name, ret);
> +		goto unlock;
> +	}
> +
> +	ovpn_tcp_send_sock_skb(sock->peer, skb);

If we didn't send the packet (because one was already queued/in
progress), we should either stash it, or tell userspace that it wasn't
sent and it should retry later.

> +	ret = size;
> +unlock:
> +	release_sock(sk);
> +peer_free:
> +	ovpn_peer_put(peer);
> +	return ret;
> +}

-- 
Sabrina