On 16/12/2024 14:59, Sabrina Dubroca wrote:
2024-12-11, 22:15:15 +0100, Antonio Quartulli wrote:
@@ -42,6 +56,31 @@ struct ovpn_peer { struct in6_addr ipv6; } vpn_addrs; struct ovpn_socket *sock;
- /* state of the TCP reading. Needed to keep track of how much of a
* single packet has already been read from the stream and how much is* missing*/nit: not so accurate since the switch to strp, can probably be dropped since @tcp has a kdoc entry
right - dropping it.
- struct {
struct strparser strp;struct work_struct tx_work;struct sk_buff_head user_queue;struct sk_buff_head out_queue;bool tx_in_progress;struct {struct sk_buff *skb;int offset;int len;} out_msg;struct {void (*sk_data_ready)(struct sock *sk);void (*sk_write_space)(struct sock *sk);struct proto *prot;const struct proto_ops *ops;} sk_cb;- } tcp;
[...]
+static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sk_buff *skb) +{
- if (peer->tcp.out_msg.skb)
ovpn_tcp_send_sock(peer);- if (peer->tcp.out_msg.skb) {
dev_core_stats_rx_dropped_inc(peer->ovpn->dev);tx_dropped?
ACK
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);
+}
+void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sk_buff *skb) +{
- u16 len = skb->len;
- *(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);
- bh_lock_sock(peer->sock->sock->sk);
- if (sock_owned_by_user(peer->sock->sock->sk)) {
if (skb_queue_len(&peer->tcp.out_queue) >=READ_ONCE(net_hotdata.max_backlog)) {dev_core_stats_rx_dropped_inc(peer->ovpn->dev);tx_dropped?
ACK
kfree_skb(skb);goto unlock;}__skb_queue_tail(&peer->tcp.out_queue, skb);- } else {
ovpn_tcp_send_sock_skb(peer, skb);- }
+unlock:
- bh_unlock_sock(peer->sock->sock->sk);
+}
[...]
+static void ovpn_tcp_close(struct sock *sk, long timeout) +{
- struct ovpn_socket *sock;
- rcu_read_lock();
[can't sleep until unlock]
- sock = rcu_dereference_sk_user_data(sk);
- strp_stop(&sock->peer->tcp.strp);
- tcp_close(sk, timeout);
void tcp_close(struct sock *sk, long timeout) { lock_sock(sk);but this can sleep.
Ouch.. I wonder why I have never seen the might_sleep() trigger this, but probably that's due to the fact that we hardly hit this cb in the classic use case.
Is there anything that prevents delaying tcp_close until after ovpn_peer_del and rcu_read_unlock?
not really.
- ovpn_peer_del(sock->peer, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR);
- rcu_read_unlock();
I will move the tcp_close() here.
+}
[...]
+void __init ovpn_tcp_init(void) +{
- ovpn_tcp_build_protos(&ovpn_tcp_prot, &ovpn_tcp_ops, &tcp_prot,
&inet_stream_ops);+#if IS_ENABLED(CONFIG_IPV6)
- ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, &tcpv6_prot,
&inet6_stream_ops);I don't think that works for CONFIG_OVPN=y and CONFIG_IPV6=m. You can either go back to the ugly thing espintcp and tls do, or use the traditional Kconfig hack:
depends on IPV6 || !IPV6
(you can find it sprinkled in various places of drivers/net/Kconfig and net/)
I'll go for the Kconfig hack. Hopefully one day IPV6 will become bool..
Thanks!
Regards,