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
