| Message ID | 20241202-b4-ovpn-v12-0-239ff733bf97@openvpn.net |
|---|---|
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
On 12/2/24 16:07, Antonio Quartulli wrote: > diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c > index 2a3dbc723813a14070159318097755cc7ea3f216..1bbbaae8639477b67626731c3bd14810a65dfdcd 100644 > --- a/drivers/net/ovpn/io.c > +++ b/drivers/net/ovpn/io.c > @@ -9,15 +9,78 @@ > > #include <linux/netdevice.h> > #include <linux/skbuff.h> > +#include <net/gro_cells.h> > #include <net/gso.h> > > -#include "io.h" > #include "ovpnstruct.h" > #include "peer.h" > +#include "io.h" > +#include "netlink.h" > +#include "proto.h" > #include "udp.h" > #include "skb.h" > #include "socket.h" > > +/* Called after decrypt to write the IP packet to the device. > + * This method is expected to manage/free the skb. > + */ > +static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb) > +{ > + unsigned int pkt_len; > + int ret; > + > + /* we can't guarantee the packet wasn't corrupted before entering the > + * VPN, therefore we give other layers a chance to check that > + */ > + skb->ip_summed = CHECKSUM_NONE; > + > + /* skb hash for transport packet no longer valid after decapsulation */ > + skb_clear_hash(skb); > + > + /* post-decrypt scrub -- prepare to inject encapsulated packet onto the > + * interface, based on __skb_tunnel_rx() in dst.h > + */ > + skb->dev = peer->ovpn->dev; > + skb_set_queue_mapping(skb, 0); > + skb_scrub_packet(skb, true); > + > + skb_reset_network_header(skb); > + skb_reset_transport_header(skb); > + skb_probe_transport_header(skb); This is a no-op after the previous call. You should drop it. Thanks, Paolo
On 03/12/2024 15:34, Paolo Abeni wrote: [...] >> +static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb) >> +{ >> + unsigned int pkt_len; >> + int ret; >> + >> + /* we can't guarantee the packet wasn't corrupted before entering the >> + * VPN, therefore we give other layers a chance to check that >> + */ >> + skb->ip_summed = CHECKSUM_NONE; >> + >> + /* skb hash for transport packet no longer valid after decapsulation */ >> + skb_clear_hash(skb); >> + >> + /* post-decrypt scrub -- prepare to inject encapsulated packet onto the >> + * interface, based on __skb_tunnel_rx() in dst.h >> + */ >> + skb->dev = peer->ovpn->dev; >> + skb_set_queue_mapping(skb, 0); >> + skb_scrub_packet(skb, true); >> + >> + skb_reset_network_header(skb); >> + skb_reset_transport_header(skb); >> + skb_probe_transport_header(skb); > > This is a no-op after the previous call. You should drop it. Thanks Paolo, I'll drop it. Regards,
On 12/2/24 16:07, Antonio Quartulli wrote: > @@ -286,6 +292,31 @@ struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_priv *ovpn, > return peer; > } > > +/** > + * ovpn_peer_check_by_src - check that skb source is routed via peer > + * @ovpn: the openvpn instance to search > + * @skb: the packet to extract source address from > + * @peer: the peer to check against the source address > + * > + * Return: true if the peer is matching or false otherwise > + */ > +bool ovpn_peer_check_by_src(struct ovpn_priv *ovpn, struct sk_buff *skb, > + struct ovpn_peer *peer) > +{ > + bool match = false; > + > + if (ovpn->mode == OVPN_MODE_P2P) { > + /* in P2P mode, no matter the destination, packets are always > + * sent to the single peer listening on the other side > + */ > + rcu_read_lock(); > + match = (peer == rcu_dereference(ovpn->peer)); > + rcu_read_unlock(); Here you are not dereferencing ovpn->peer, so you can use rcu_access_pointer() instead and avoid the rcu_read_lock/unlock() pair. /P
On 12/2/24 16:07, Antonio Quartulli wrote: > +void ovpn_tcp_socket_detach(struct socket *sock) > +{ > + struct ovpn_socket *ovpn_sock; > + struct ovpn_peer *peer; > + > + if (!sock) > + return; > + > + rcu_read_lock(); > + ovpn_sock = rcu_dereference_sk_user_data(sock->sk); > + > + if (!ovpn_sock->peer) { > + rcu_read_unlock(); > + return; > + } > + > + peer = ovpn_sock->peer; > + strp_stop(&peer->tcp.strp); > + > + skb_queue_purge(&peer->tcp.user_queue); > + > + /* 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; > + /* drop reference to peer */ > + rcu_assign_sk_user_data(sock->sk, NULL); > + > + rcu_read_unlock(); > + > + barrier(); It's unclear to me the role of the above barrier. A comment would help > + /* cancel any ongoing work. Done after removing the CBs so that these > + * workers cannot be re-armed > + */ > + cancel_work_sync(&peer->tcp.tx_work); > + strp_done(&peer->tcp.strp); > + skb_queue_purge(&peer->tcp.out_queue); > + > + ovpn_peer_put(peer); > +} > + > +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; > + > + 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", > + 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); > + 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); > +} > + > +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); > + 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); Are you sure this runs in BH context? AFAICS we reach here from an AEAD callback. > + 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); > + 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_build_protos(struct proto *new_prot, > + struct proto_ops *new_ops, > + const struct proto *orig_prot, > + const struct proto_ops *orig_ops); > + > +/* Set TCP encapsulation callbacks */ > +int ovpn_tcp_socket_attach(struct socket *sock, struct ovpn_peer *peer) > +{ > + struct strp_callbacks cb = { > + .rcv_msg = ovpn_tcp_rcv, > + .parse_msg = ovpn_tcp_parse, > + }; > + int ret; > + > + /* make sure no pre-existing encapsulation handler exists */ > + if (sock->sk->sk_user_data) > + return -EBUSY; > + > + /* sanity check */ > + if (sock->sk->sk_protocol != IPPROTO_TCP) { > + net_err_ratelimited("%s: provided socket is not TCP as expected\n", > + netdev_name(peer->ovpn->dev)); > + return -EINVAL; > + } > + > + /* only a fully connected socket are expected. Connection should be > + * handled in userspace > + */ > + if (sock->sk->sk_state != TCP_ESTABLISHED) { > + net_err_ratelimited("%s: provided TCP socket is not in ESTABLISHED state: %d\n", > + netdev_name(peer->ovpn->dev), > + sock->sk->sk_state); > + return -EINVAL; > + } > + > + lock_sock(sock->sk); > + > + ret = strp_init(&peer->tcp.strp, sock->sk, &cb); > + if (ret < 0) { > + DEBUG_NET_WARN_ON_ONCE(1); > + release_sock(sock->sk); > + return ret; > + } > + > + INIT_WORK(&peer->tcp.tx_work, ovpn_tcp_tx_work); > + __sk_dst_reset(sock->sk); > + skb_queue_head_init(&peer->tcp.user_queue); > + skb_queue_head_init(&peer->tcp.out_queue); > + > + /* save current CBs so that they can be restored upon socket release */ > + peer->tcp.sk_cb.sk_data_ready = sock->sk->sk_data_ready; > + peer->tcp.sk_cb.sk_write_space = sock->sk->sk_write_space; > + peer->tcp.sk_cb.prot = sock->sk->sk_prot; > + peer->tcp.sk_cb.ops = sock->sk->sk_socket->ops; > + > + /* assign our static CBs and prot/ops */ > + sock->sk->sk_data_ready = ovpn_tcp_data_ready; > + sock->sk->sk_write_space = ovpn_tcp_write_space; > + > + if (sock->sk->sk_family == AF_INET) { > + sock->sk->sk_prot = &ovpn_tcp_prot; > + sock->sk->sk_socket->ops = &ovpn_tcp_ops; > + } else { > + mutex_lock(&tcp6_prot_mutex); > + if (!ovpn_tcp6_prot.recvmsg) > + ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, > + sock->sk->sk_prot, > + sock->sk->sk_socket->ops); > + mutex_unlock(&tcp6_prot_mutex); This looks like an hack to avoid a build dependency on IPV6, I think the explicit #if IS_ENABLED(CONFIG_IPV6) at init time should be preferable > + > + sock->sk->sk_prot = &ovpn_tcp6_prot; > + sock->sk->sk_socket->ops = &ovpn_tcp6_ops; > + } [...] > +static void ovpn_tcp_close(struct sock *sk, long timeout) > +{ > + struct ovpn_socket *sock; > + > + rcu_read_lock(); > + sock = rcu_dereference_sk_user_data(sk); > + > + strp_stop(&sock->peer->tcp.strp); > + barrier(); Again, is not clear to me the role of the above barrier, please document it. Thanks, Paolo
2024-12-03, 15:58:17 +0100, Antonio Quartulli wrote: > On 02/12/2024 16:07, Antonio Quartulli wrote: > [...] > > +#define ovpn_get_hash_slot(_key, _key_len, _tbl) ({ \ > > + typeof(_tbl) *__tbl = &(_tbl); \ > > + jhash(_key, _key_len, 0) % HASH_SIZE(*__tbl); \ > > +}) > > + > > +#define ovpn_get_hash_head(_tbl, _key, _key_len) ({ \ > > + typeof(_tbl) *__tbl = &(_tbl); \ > > + &(*__tbl)[ovpn_get_hash_slot(_key, _key_len, *__tbl)]; \ > > +}) > > clang a reporting various warnings like this: > > ../drivers/net/ovpn/peer.c:406:9: warning: variable '__tbl' is uninitialized > when used within its own initialization [-Wuninitialized] > 406 | head = ovpn_get_hash_head(ovpn->peers->by_id, &peer_id, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 407 | sizeof(peer_id)); > | ~~~~~~~~~~~~~~~~ > ../drivers/net/ovpn/peer.c:179:48: note: expanded from macro > 'ovpn_get_hash_head' > 179 | &(*__tbl)[ovpn_get_hash_slot(_key, _key_len, *__tbl)]; \ > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~ > ../drivers/net/ovpn/peer.c:173:26: note: expanded from macro > 'ovpn_get_hash_slot' > 173 | typeof(_tbl) *__tbl = &(_tbl); \ > | ~~~~~ ^~~~ > > Anybody willing to help me understand this issue? > > I have troubles figuring out how __tbl is being used uninitialized. > I wonder if the parameters naming is fooling clang (or me) somehow. Not really a solution to this specific issue, but do you actually need ovpn_get_hash_slot as a separate macro? AFAICT all users could also be converted to ovpn_get_hash_head, then you can merge ovpn_get_hash_slot into ovpn_get_hash_head and maybe clang won't get confused? No guarantee that this fixes anything (except saving one or two lines in a few functions).
On 04/12/2024 09:28, Antonio Quartulli wrote: > On 03/12/2024 17:09, Sabrina Dubroca wrote: >> 2024-12-03, 15:58:17 +0100, Antonio Quartulli wrote: >>> On 02/12/2024 16:07, Antonio Quartulli wrote: >>> [...] >>>> +#define ovpn_get_hash_slot(_key, _key_len, _tbl) ({ \ >>>> + typeof(_tbl) *__tbl = &(_tbl); \ >>>> + jhash(_key, _key_len, 0) % HASH_SIZE(*__tbl); \ >>>> +}) >>>> + >>>> +#define ovpn_get_hash_head(_tbl, _key, _key_len) ({ \ >>>> + typeof(_tbl) *__tbl = &(_tbl); \ >>>> + &(*__tbl)[ovpn_get_hash_slot(_key, _key_len, *__tbl)]; \ >>>> +}) >>> >>> clang a reporting various warnings like this: >>> >>> ../drivers/net/ovpn/peer.c:406:9: warning: variable '__tbl' is >>> uninitialized >>> when used within its own initialization [-Wuninitialized] >>> 406 | head = ovpn_get_hash_head(ovpn->peers->by_id, &peer_id, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> 407 | sizeof(peer_id)); >>> | ~~~~~~~~~~~~~~~~ >>> ../drivers/net/ovpn/peer.c:179:48: note: expanded from macro >>> 'ovpn_get_hash_head' >>> 179 | &(*__tbl)[ovpn_get_hash_slot(_key, _key_len, >>> *__tbl)]; \ >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~ >>> ../drivers/net/ovpn/peer.c:173:26: note: expanded from macro >>> 'ovpn_get_hash_slot' >>> 173 | typeof(_tbl) *__tbl = &(_tbl); \ >>> | ~~~~~ ^~~~ >>> >>> Anybody willing to help me understand this issue? >>> >>> I have troubles figuring out how __tbl is being used uninitialized. >>> I wonder if the parameters naming is fooling clang (or me) somehow. >> >> Not really a solution to this specific issue, but do you actually need >> ovpn_get_hash_slot as a separate macro? AFAICT all users could also be >> converted to ovpn_get_hash_head, then you can merge ovpn_get_hash_slot >> into ovpn_get_hash_head and maybe clang won't get confused? >> >> No guarantee that this fixes anything (except saving one or two lines >> in a few functions). > > This is what it used to be before (and no error was reported), but I had > to split the macro because I need to isolate the slot computation for > nulls comparison. So there are some users for ovpn_get_hash_slot() > > I will quickly try changing the naming and see if clang gets happier. Indeed it's the declaration of __tbl in ovpn_get_hash_slot() that confuses clang. I'll rename both __tbl and add a comment to remember why we did that. Regards, > > Regards, > >
On 04/12/2024 12:15, Antonio Quartulli wrote: [...] >>> +static void ovpn_tcp_close(struct sock *sk, long timeout) >>> +{ >>> + struct ovpn_socket *sock; >>> + >>> + rcu_read_lock(); >>> + sock = rcu_dereference_sk_user_data(sk); >>> + >>> + strp_stop(&sock->peer->tcp.strp); >>> + barrier(); >> >> Again, is not clear to me the role of the above barrier, please >> document it. > > Also taken from espintcp_close(), with the idea to avoid reordering > during the shut down sequence. > > Will add a comment here too. Actually, after checking this specific barrier(), I realized this is not needed, because we are doing the socket detach later, after calling the ovpn_peer_del() (which is different from what is happening in espintcp). So I'll just drop this barrier here. Regards,
Paolo, On 04/12/2024 12:15, Antonio Quartulli wrote: [...] >>> + mutex_lock(&tcp6_prot_mutex); >>> + if (!ovpn_tcp6_prot.recvmsg) >>> + ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, >>> + sock->sk->sk_prot, >>> + sock->sk->sk_socket->ops); >>> + mutex_unlock(&tcp6_prot_mutex); >> >> This looks like an hack to avoid a build dependency on IPV6, I think the >> explicit > > I happily copied this approach from espintcp.c:espintcp_init_sk() :-D > >> >> #if IS_ENABLED(CONFIG_IPV6) >> >> at init time should be preferable To get this done at init time I need inet6_stream_ops to be accessible, but it seems there is no EXPORT_SYMBOL() for this object. However, I see that mptcp/protocol.c is happily accessing it. Any clue how this is possible? If I try to access it within ovpn I get the usual error: ERROR: modpost: "inet6_stream_ops" [drivers/net/ovpn/ovpn.ko] undefined! but mptcp compiles just fine. Any hint? Regards,
On 04/12/2024 23:52, Antonio Quartulli wrote: > Paolo, > > On 04/12/2024 12:15, Antonio Quartulli wrote: > [...] >>>> + mutex_lock(&tcp6_prot_mutex); >>>> + if (!ovpn_tcp6_prot.recvmsg) >>>> + ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, >>>> + sock->sk->sk_prot, >>>> + sock->sk->sk_socket->ops); >>>> + mutex_unlock(&tcp6_prot_mutex); >>> >>> This looks like an hack to avoid a build dependency on IPV6, I think the >>> explicit >> >> I happily copied this approach from espintcp.c:espintcp_init_sk() :-D >> >>> >>> #if IS_ENABLED(CONFIG_IPV6) >>> >>> at init time should be preferable > > To get this done at init time I need inet6_stream_ops to be accessible, > but it seems there is no EXPORT_SYMBOL() for this object. > > However, I see that mptcp/protocol.c is happily accessing it. > Any clue how this is possible? I answer myself: mptcp is not tristate and it can only be compiled as built-in. If I compile ovpn as built-in I also get everything built fine. Paolo, do you have any recommendation? Should I patch net/ipv6/af_inet6.c and add the missing EXPORT_SYMBOL()? Regards,
Hi Antonio, Thank you for working on this, and sharing your work here! On 05/12/2024 00:09, Antonio Quartulli wrote: > On 04/12/2024 23:52, Antonio Quartulli wrote: >> Paolo, >> >> On 04/12/2024 12:15, Antonio Quartulli wrote: >> [...] >>>>> + mutex_lock(&tcp6_prot_mutex); >>>>> + if (!ovpn_tcp6_prot.recvmsg) >>>>> + ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, >>>>> + sock->sk->sk_prot, >>>>> + sock->sk->sk_socket->ops); >>>>> + mutex_unlock(&tcp6_prot_mutex); >>>> >>>> This looks like an hack to avoid a build dependency on IPV6, I think >>>> the >>>> explicit >>> >>> I happily copied this approach from espintcp.c:espintcp_init_sk() :-D >>> >>>> >>>> #if IS_ENABLED(CONFIG_IPV6) >>>> >>>> at init time should be preferable >> >> To get this done at init time I need inet6_stream_ops to be >> accessible, but it seems there is no EXPORT_SYMBOL() for this object. >> >> However, I see that mptcp/protocol.c is happily accessing it. >> Any clue how this is possible? > > I answer myself: mptcp is not tristate and it can only be compiled as > built-in. Indeed, that's why. Talking about MPTCP, by chance, do you plan to support it later on? :) Cheers, Matt
On 09/12/2024 11:46, Matthieu Baerts wrote: > Hi Antonio, > > Thank you for working on this, and sharing your work here! > > On 05/12/2024 00:09, Antonio Quartulli wrote: >> On 04/12/2024 23:52, Antonio Quartulli wrote: >>> Paolo, >>> >>> On 04/12/2024 12:15, Antonio Quartulli wrote: >>> [...] >>>>>> + mutex_lock(&tcp6_prot_mutex); >>>>>> + if (!ovpn_tcp6_prot.recvmsg) >>>>>> + ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, >>>>>> + sock->sk->sk_prot, >>>>>> + sock->sk->sk_socket->ops); >>>>>> + mutex_unlock(&tcp6_prot_mutex); >>>>> >>>>> This looks like an hack to avoid a build dependency on IPV6, I think >>>>> the >>>>> explicit >>>> >>>> I happily copied this approach from espintcp.c:espintcp_init_sk() :-D >>>> >>>>> >>>>> #if IS_ENABLED(CONFIG_IPV6) >>>>> >>>>> at init time should be preferable >>> >>> To get this done at init time I need inet6_stream_ops to be >>> accessible, but it seems there is no EXPORT_SYMBOL() for this object. >>> >>> However, I see that mptcp/protocol.c is happily accessing it. >>> Any clue how this is possible? >> >> I answer myself: mptcp is not tristate and it can only be compiled as >> built-in. > > Indeed, that's why. > > Talking about MPTCP, by chance, do you plan to support it later on? :) Hi Matthieu, It is not on our current roadmap (TCP doesn't get much love in the VPN world), but I agree it could be an interesting option to explore! I have to admit that I haven't played much with MPTCP myself yet, but I am more than happy to talk about potential advantages for the ovpn use case. Cheers,
On 09/12/2024 11:58, Antonio Quartulli wrote: > On 09/12/2024 11:46, Matthieu Baerts wrote: >> Hi Antonio, >> >> Thank you for working on this, and sharing your work here! >> >> On 05/12/2024 00:09, Antonio Quartulli wrote: >>> On 04/12/2024 23:52, Antonio Quartulli wrote: >>>> Paolo, >>>> >>>> On 04/12/2024 12:15, Antonio Quartulli wrote: >>>> [...] >>>>>>> + mutex_lock(&tcp6_prot_mutex); >>>>>>> + if (!ovpn_tcp6_prot.recvmsg) >>>>>>> + ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, >>>>>>> + sock->sk->sk_prot, >>>>>>> + sock->sk->sk_socket->ops); >>>>>>> + mutex_unlock(&tcp6_prot_mutex); >>>>>> >>>>>> This looks like an hack to avoid a build dependency on IPV6, I think >>>>>> the >>>>>> explicit >>>>> >>>>> I happily copied this approach from espintcp.c:espintcp_init_sk() :-D >>>>> >>>>>> >>>>>> #if IS_ENABLED(CONFIG_IPV6) >>>>>> >>>>>> at init time should be preferable >>>> >>>> To get this done at init time I need inet6_stream_ops to be >>>> accessible, but it seems there is no EXPORT_SYMBOL() for this object. >>>> >>>> However, I see that mptcp/protocol.c is happily accessing it. >>>> Any clue how this is possible? >>> >>> I answer myself: mptcp is not tristate and it can only be compiled as >>> built-in. >> >> Indeed, that's why. >> >> Talking about MPTCP, by chance, do you plan to support it later on? :) > > Hi Matthieu, > > It is not on our current roadmap (TCP doesn't get much love in the VPN > world), but I agree it could be an interesting option to explore! I understand, it makes sense not to recommend using TCP for the transport layer for tunnelling solutions. > I have to admit that I haven't played much with MPTCP myself yet, but I > am more than happy to talk about potential advantages for the ovpn use > case. Some people told me they were interested in using OpenVPN with MPTCP to use multiple (low-capacity) network links at the same time. I think intercepting and proxying TCP traffic would always be the best in terms of performances, but using OpenVPN with MPTCP seems to be enough for some, especially when they want to "improve" some type of UDP traffic that cannot be intercepted: QUIC, VPN, etc. I don't have numbers to share, but I can understand this feature can help in some cases. (This reminds me this: https://github.com/OpenVPN/ovpn-dco/issues/60) (and this: https://github.com/arinc9/openvpn/pull/1) Cheers, Matt
On 09/12/2024 12:31, Matthieu Baerts wrote: > On 09/12/2024 11:58, Antonio Quartulli wrote: >> On 09/12/2024 11:46, Matthieu Baerts wrote: >>> Hi Antonio, >>> >>> Thank you for working on this, and sharing your work here! >>> >>> On 05/12/2024 00:09, Antonio Quartulli wrote: >>>> On 04/12/2024 23:52, Antonio Quartulli wrote: >>>>> Paolo, >>>>> >>>>> On 04/12/2024 12:15, Antonio Quartulli wrote: >>>>> [...] >>>>>>>> + mutex_lock(&tcp6_prot_mutex); >>>>>>>> + if (!ovpn_tcp6_prot.recvmsg) >>>>>>>> + ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, >>>>>>>> + sock->sk->sk_prot, >>>>>>>> + sock->sk->sk_socket->ops); >>>>>>>> + mutex_unlock(&tcp6_prot_mutex); >>>>>>> >>>>>>> This looks like an hack to avoid a build dependency on IPV6, I think >>>>>>> the >>>>>>> explicit >>>>>> >>>>>> I happily copied this approach from espintcp.c:espintcp_init_sk() :-D >>>>>> >>>>>>> >>>>>>> #if IS_ENABLED(CONFIG_IPV6) >>>>>>> >>>>>>> at init time should be preferable >>>>> >>>>> To get this done at init time I need inet6_stream_ops to be >>>>> accessible, but it seems there is no EXPORT_SYMBOL() for this object. >>>>> >>>>> However, I see that mptcp/protocol.c is happily accessing it. >>>>> Any clue how this is possible? >>>> >>>> I answer myself: mptcp is not tristate and it can only be compiled as >>>> built-in. >>> >>> Indeed, that's why. >>> >>> Talking about MPTCP, by chance, do you plan to support it later on? :) >> >> Hi Matthieu, >> >> It is not on our current roadmap (TCP doesn't get much love in the VPN >> world), but I agree it could be an interesting option to explore! > > I understand, it makes sense not to recommend using TCP for the > transport layer for tunnelling solutions. > >> I have to admit that I haven't played much with MPTCP myself yet, but I >> am more than happy to talk about potential advantages for the ovpn use >> case. > > Some people told me they were interested in using OpenVPN with MPTCP to > use multiple (low-capacity) network links at the same time. I think > intercepting and proxying TCP traffic would always be the best in terms > of performances, but using OpenVPN with MPTCP seems to be enough for > some, especially when they want to "improve" some type of UDP traffic > that cannot be intercepted: QUIC, VPN, etc. > > I don't have numbers to share, but I can understand this feature can > help in some cases. Yeah, some people may definitely benefit from this feature. I'll have a look at MPTCP once ovpn is merged. > > (This reminds me this: https://github.com/OpenVPN/ovpn-dco/issues/60) > (and this: https://github.com/arinc9/openvpn/pull/1) Right, this definitely shows some interest and it means we should easily find people willing to test :-) Regards,
On 09/12/2024 15:08, Antonio Quartulli wrote: > On 09/12/2024 12:31, Matthieu Baerts wrote: >> On 09/12/2024 11:58, Antonio Quartulli wrote: >>> On 09/12/2024 11:46, Matthieu Baerts wrote: >>>> Hi Antonio, >>>> >>>> Thank you for working on this, and sharing your work here! >>>> >>>> On 05/12/2024 00:09, Antonio Quartulli wrote: >>>>> On 04/12/2024 23:52, Antonio Quartulli wrote: >>>>>> Paolo, >>>>>> >>>>>> On 04/12/2024 12:15, Antonio Quartulli wrote: >>>>>> [...] >>>>>>>>> + mutex_lock(&tcp6_prot_mutex); >>>>>>>>> + if (!ovpn_tcp6_prot.recvmsg) >>>>>>>>> + ovpn_tcp_build_protos(&ovpn_tcp6_prot, >>>>>>>>> &ovpn_tcp6_ops, >>>>>>>>> + sock->sk->sk_prot, >>>>>>>>> + sock->sk->sk_socket->ops); >>>>>>>>> + mutex_unlock(&tcp6_prot_mutex); >>>>>>>> >>>>>>>> This looks like an hack to avoid a build dependency on IPV6, I >>>>>>>> think >>>>>>>> the >>>>>>>> explicit >>>>>>> >>>>>>> I happily copied this approach from >>>>>>> espintcp.c:espintcp_init_sk() :-D >>>>>>> >>>>>>>> >>>>>>>> #if IS_ENABLED(CONFIG_IPV6) >>>>>>>> >>>>>>>> at init time should be preferable >>>>>> >>>>>> To get this done at init time I need inet6_stream_ops to be >>>>>> accessible, but it seems there is no EXPORT_SYMBOL() for this object. >>>>>> >>>>>> However, I see that mptcp/protocol.c is happily accessing it. >>>>>> Any clue how this is possible? >>>>> >>>>> I answer myself: mptcp is not tristate and it can only be compiled as >>>>> built-in. >>>> >>>> Indeed, that's why. >>>> >>>> Talking about MPTCP, by chance, do you plan to support it later on? :) >>> >>> Hi Matthieu, >>> >>> It is not on our current roadmap (TCP doesn't get much love in the VPN >>> world), but I agree it could be an interesting option to explore! >> >> I understand, it makes sense not to recommend using TCP for the >> transport layer for tunnelling solutions. >> >>> I have to admit that I haven't played much with MPTCP myself yet, but I >>> am more than happy to talk about potential advantages for the ovpn use >>> case. >> >> Some people told me they were interested in using OpenVPN with MPTCP to >> use multiple (low-capacity) network links at the same time. I think >> intercepting and proxying TCP traffic would always be the best in terms >> of performances, but using OpenVPN with MPTCP seems to be enough for >> some, especially when they want to "improve" some type of UDP traffic >> that cannot be intercepted: QUIC, VPN, etc. >> >> I don't have numbers to share, but I can understand this feature can >> help in some cases. > > Yeah, some people may definitely benefit from this feature. > I'll have a look at MPTCP once ovpn is merged. Thank you :) Don't hesitate to email the ML, or open an issue on the GitHub repo if needed! (More details: https://www.mptcp.dev/#communication) >> (This reminds me this: https://github.com/OpenVPN/ovpn-dco/issues/60) >> (and this: https://github.com/arinc9/openvpn/pull/1) > > Right, this definitely shows some interest and it means we should easily > find people willing to test :-) Indeed! Cheers, Matt
This is the 12th version of the patchset. Hopefully there are no major flaws that will require more resendings. I am sure we'll have plenty of time to polish up all bells and whistles :-) @Sergey, at the end I think I took in all your suggested changes, maybe with some adaptations. Notable changes from v11: * move 'select' entries in Kconfig from patch 1 to where those deps are used * mark mailing list as subscribers-only in MAINTAINERS file * check iface validity against net_device_ops instead of ndo_start_xmit * drop DRV_ defines in favour of literals * use "ovpn" literal instead of OVPN_FAMILY_NAME in code that is not netlink related * delete all peers on ifdown (new del-peer reason added accordingly) * don't allow adding new peers if iface is down * clarified uniqueness of IDs in netlink spec * renamed ovpn_struct to ovpn_priv * removed packet.h and moved content to proto.h * fixed overhead/head_room calculation * dropped unused ovpn_priv.dev_list member * ensured all defines are prefixed with OVPN_ * kept carrier on only for MP mode * carrier in P2P mode goes on/off when peer is added/deleted * dropped skb_protocol_to_family() in favour of checking skb->protocol directly * dropped ovpn_priv.peers.lock in favour of ovpn_priv.lock * dropped error message in case of packet with unknown ID * dropped sanity check in udp socket attach function * made ovpn_peer_skb_to_sockaddr() return sockaddr len to simplify code * dropped __must_hold() in favour of lockdep_assert_held() * with TCP patch ovpn_socket now holds reference to ovpn_priv (UDP) or ovpn_peer (TCP) to prevent use-after-free of peer in TCP code and to force cleanup code to wait for TCP scheduled work * ovpn_peer release refactored in two steps to allow implementing previous point (reference to socket is now dropped in first step, instead of kref callback) * dropped all mentions of __func__ in messages * moved introduction of UDP_ENCAP_OVPNINUDP from patch 1 to related patch * properly update vpn and link statistics at right time instead of same spot * properly checked skb head size before accessing ipv6 header in ovpn_ip_check_protocol() * merged ovpn_peer_update_local_endpoint() and ovpn_peer_float() * properly locked peer collection when rehashing upon peer float * used netdev_name() when possible for printing iface name * destroyed dst_cache only upon final peer release * used bitfield APIs for opcode parsing and creation * dropped struct ovpn_nonce_tail in favour of using u8[] directly * added comment about skb_reset_network_header() placement * added locking around peer->bind modifications * added TCP out_queue to stash data skbs when socket is owned by user (to be sent out upon sock release) * added call to barrier() in TCP socket release * fixed hlist nulls lookup by adding loop restart * used WRITE/READ_ONCE with last_recv/sent * stopped counting keepalive msgs as dropped packets * improved ovpn_nl_peer_precheck() to account for mixed v4mapped IPv6 * rehash peer after PEER_SET only in MP mode addresses * added iface teardown check to kselftest script * Link to v11: https://lore.kernel.org/r/20241029-b4-ovpn-v11-0-de4698c73a25@openvpn.net Please note that some patches were already reviewed by Andre Lunn, Donald Hunter and Shuah Khan. They have retained the Reviewed-by tag since no major code modification has happened since the review. Patch The latest code can also be found at: https://github.com/OpenVPN/linux-kernel-ovpn Thanks a lot! Best Regards, Antonio Quartulli OpenVPN Inc. --- Antonio Quartulli (22): net: introduce OpenVPN Data Channel Offload (ovpn) ovpn: add basic netlink support ovpn: add basic interface creation/destruction/management routines ovpn: keep carrier always on for MP interfaces ovpn: introduce the ovpn_peer object ovpn: introduce the ovpn_socket object ovpn: implement basic TX path (UDP) ovpn: implement basic RX path (UDP) ovpn: implement packet processing ovpn: store tunnel and transport statistics ovpn: implement TCP transport ovpn: implement multi-peer support ovpn: implement peer lookup logic ovpn: implement keepalive mechanism ovpn: add support for updating local UDP endpoint ovpn: add support for peer floating ovpn: implement peer add/get/dump/delete via netlink ovpn: implement key add/get/del/swap via netlink ovpn: kill key and notify userspace in case of IV exhaustion ovpn: notify userspace when a peer is deleted ovpn: add basic ethtool support testing/selftests: add test tool and scripts for ovpn module Documentation/netlink/specs/ovpn.yaml | 368 +++ MAINTAINERS | 11 + drivers/net/Kconfig | 14 + drivers/net/Makefile | 1 + drivers/net/ovpn/Makefile | 22 + drivers/net/ovpn/bind.c | 55 + drivers/net/ovpn/bind.h | 101 + drivers/net/ovpn/crypto.c | 211 ++ drivers/net/ovpn/crypto.h | 145 ++ drivers/net/ovpn/crypto_aead.c | 383 ++++ drivers/net/ovpn/crypto_aead.h | 33 + drivers/net/ovpn/io.c | 446 ++++ drivers/net/ovpn/io.h | 34 + drivers/net/ovpn/main.c | 339 +++ drivers/net/ovpn/main.h | 14 + drivers/net/ovpn/netlink-gen.c | 212 ++ drivers/net/ovpn/netlink-gen.h | 41 + drivers/net/ovpn/netlink.c | 1178 ++++++++++ drivers/net/ovpn/netlink.h | 18 + drivers/net/ovpn/ovpnstruct.h | 57 + drivers/net/ovpn/peer.c | 1278 +++++++++++ drivers/net/ovpn/peer.h | 163 ++ drivers/net/ovpn/pktid.c | 129 ++ drivers/net/ovpn/pktid.h | 87 + drivers/net/ovpn/proto.h | 118 + drivers/net/ovpn/skb.h | 58 + drivers/net/ovpn/socket.c | 180 ++ drivers/net/ovpn/socket.h | 55 + drivers/net/ovpn/stats.c | 21 + drivers/net/ovpn/stats.h | 47 + drivers/net/ovpn/tcp.c | 579 +++++ drivers/net/ovpn/tcp.h | 33 + drivers/net/ovpn/udp.c | 397 ++++ drivers/net/ovpn/udp.h | 23 + include/uapi/linux/if_link.h | 15 + include/uapi/linux/ovpn.h | 110 + include/uapi/linux/udp.h | 1 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/net/ovpn/.gitignore | 2 + tools/testing/selftests/net/ovpn/Makefile | 17 + tools/testing/selftests/net/ovpn/config | 10 + tools/testing/selftests/net/ovpn/data64.key | 5 + tools/testing/selftests/net/ovpn/ovpn-cli.c | 2370 ++++++++++++++++++++ tools/testing/selftests/net/ovpn/tcp_peers.txt | 5 + .../testing/selftests/net/ovpn/test-chachapoly.sh | 9 + tools/testing/selftests/net/ovpn/test-float.sh | 9 + tools/testing/selftests/net/ovpn/test-tcp.sh | 9 + tools/testing/selftests/net/ovpn/test.sh | 182 ++ tools/testing/selftests/net/ovpn/udp_peers.txt | 5 + 49 files changed, 9601 insertions(+) --- base-commit: 65ae975e97d5aab3ee9dc5ec701b12090572ed43 change-id: 20241002-b4-ovpn-eeee35c694a2 Best regards,