Re: [PATCH net-next v20 12/25] ovpn: implement TCP transport

On 02/03/2025 19:59, Sabrina Dubroca wrote:

2025-02-27, 02:21:37 +0100, Antonio Quartulli wrote:

quoted

Moreover export tcp_release_cb by means of EXPORT_SYMBOL instead of
EXPORT_IPV6_MOD, so that other modules can use it, even if IPV6 is
not compiled in.

Is that really needed? You're saving tcp.sk_cb.prot, so you could just
call peer->tcp.sk_cb.prot->release_cb? (with a bit of care since it's
called after peer_put)

[I don't know what the maintainers' preference is wrt "re-exporting"
symbols that got moved to EXPORT_IPV6_MOD]

Mh yeah, I like your suggestion as I don't have to change the EXPORT and also I don't have to make assumptions on the original value of ->release_cb as I can just call it.

Will follow your suggestion. Thanks!

[...]

quoted

+static void ovpn_tcp_send_sock(struct ovpn_peer *peer, struct sock *sk)
+{
+	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;
+
+	do {
+		int ret = skb_send_sock_locked(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",
+					     netdev_name(peer->ovpn->dev),
+					     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);

I don't think this works:

ovpn_peer_del -> unlock_ovpn -> ovpn_socket_release -> might_sleep

but we can get to ovpn_tcp_send_sock in a few contexts that are not
allowed to sleep:

ovpn_tcp_send_skb -> ovpn_tcp_send_sock_skb -> ovpn_tcp_send_sock
__sk_flush_backlog -> release_cb = ovpn_tcp_release -> ovpn_tcp_send_sock_skb
release_sock       -> release_cb = ovpn_tcp_release -> ovpn_tcp_send_sock_skb


(I checked all other paths leading to unlock_ovpn/ovpn_socket_release,
this is the only one I could find that is not allowed to sleep. So it
would likely be easier to push this peer_del (or even just the
handling of release_list) into some other sleepable context than
trying to reshuffle all the other paths)

ACK (and thanks for double checking all other paths!)
I will do some magic on this peer_del() in order to make it sleep without issues.

Thanks!

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.