| Message ID | 20241029-b4-ovpn-v11-0-de4698c73a25@openvpn.net |
|---|---|
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote: > +static void ovpn_peer_release(struct ovpn_peer *peer) > +{ > + ovpn_bind_reset(peer, NULL); > + > + dst_cache_destroy(&peer->dst_cache); Is it safe to destroy the cache at this time? In the same function, we use rcu to free the peer, but AFAICT the dst_cache will be freed immediately: void dst_cache_destroy(struct dst_cache *dst_cache) { [...] free_percpu(dst_cache->cache); } (probably no real issue because ovpn_udp_send_skb gets called while we hold a reference to the peer?) > + netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker); > + kfree_rcu(peer, rcu); > +} [...] > +static int ovpn_peer_del_p2p(struct ovpn_peer *peer, > + enum ovpn_del_peer_reason reason) > + __must_hold(&peer->ovpn->lock) > +{ > + struct ovpn_peer *tmp; > + > + tmp = rcu_dereference_protected(peer->ovpn->peer, > + lockdep_is_held(&peer->ovpn->lock)); > + if (tmp != peer) { > + DEBUG_NET_WARN_ON_ONCE(1); > + if (tmp) > + ovpn_peer_put(tmp); Does peer->ovpn->peer need to be set to NULL here as well? Or is it going to survive this _put? > + > + return -ENOENT; > + } > + > + tmp->delete_reason = reason; > + RCU_INIT_POINTER(peer->ovpn->peer, NULL); > + ovpn_peer_put(tmp); > + > + return 0; > +}
On 30/10/2024 17:37, Sabrina Dubroca wrote: > 2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote: >> +static void ovpn_peer_release(struct ovpn_peer *peer) >> +{ >> + ovpn_bind_reset(peer, NULL); >> + >> + dst_cache_destroy(&peer->dst_cache); > > Is it safe to destroy the cache at this time? In the same function, we > use rcu to free the peer, but AFAICT the dst_cache will be freed > immediately: > > void dst_cache_destroy(struct dst_cache *dst_cache) > { > [...] > free_percpu(dst_cache->cache); > } > > (probably no real issue because ovpn_udp_send_skb gets called while we > hold a reference to the peer?) Right. That was my assumption: release happens on refcnt = 0 only, therefore no field should be in use anymore. Anything that may still be in use will have its own refcounter. > >> + netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker); >> + kfree_rcu(peer, rcu); >> +} > > > [...] >> +static int ovpn_peer_del_p2p(struct ovpn_peer *peer, >> + enum ovpn_del_peer_reason reason) >> + __must_hold(&peer->ovpn->lock) >> +{ >> + struct ovpn_peer *tmp; >> + >> + tmp = rcu_dereference_protected(peer->ovpn->peer, >> + lockdep_is_held(&peer->ovpn->lock)); >> + if (tmp != peer) { >> + DEBUG_NET_WARN_ON_ONCE(1); >> + if (tmp) >> + ovpn_peer_put(tmp); > > Does peer->ovpn->peer need to be set to NULL here as well? Or is it > going to survive this _put? First of all consider that this is truly something that we don't expect to happen (hence the WARN_ON). If this is happening it's because we are trying to delete a peer that is not the one we are connected to (unexplainable scenario in p2p mode). Still, should we hit this case (I truly can't see how), I'd say "leave everything as is - maybe this call was just a mistake". Cheers, > >> + >> + return -ENOENT; >> + } >> + >> + tmp->delete_reason = reason; >> + RCU_INIT_POINTER(peer->ovpn->peer, NULL); >> + ovpn_peer_put(tmp); >> + >> + return 0; >> +} >
It seems some little changes to ovpn.yaml were not reflected in the generated files I committed. Specifically I changed some U32 to BE32 (IPv4 addresses) and files were not regenerated before committing. (I saw the failure in patchwork about this) It seems I'll have to send v12 after all. Cheers, On 29/10/2024 11:47, Antonio Quartulli wrote: > Notable changes from v10: > * extended commit message of 23/23 with brief description of the output > * Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0-b87530777be7@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. > > 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 (23): > netlink: add NLA_POLICY_MAX_LEN macro > 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 > 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 | 362 +++ > MAINTAINERS | 11 + > drivers/net/Kconfig | 14 + > drivers/net/Makefile | 1 + > drivers/net/ovpn/Makefile | 22 + > drivers/net/ovpn/bind.c | 54 + > drivers/net/ovpn/bind.h | 117 + > drivers/net/ovpn/crypto.c | 214 ++ > drivers/net/ovpn/crypto.h | 145 ++ > drivers/net/ovpn/crypto_aead.c | 386 ++++ > drivers/net/ovpn/crypto_aead.h | 33 + > drivers/net/ovpn/io.c | 462 ++++ > drivers/net/ovpn/io.h | 25 + > drivers/net/ovpn/main.c | 337 +++ > drivers/net/ovpn/main.h | 24 + > drivers/net/ovpn/netlink-gen.c | 212 ++ > drivers/net/ovpn/netlink-gen.h | 41 + > drivers/net/ovpn/netlink.c | 1135 ++++++++++ > drivers/net/ovpn/netlink.h | 18 + > drivers/net/ovpn/ovpnstruct.h | 61 + > drivers/net/ovpn/packet.h | 40 + > drivers/net/ovpn/peer.c | 1201 ++++++++++ > drivers/net/ovpn/peer.h | 165 ++ > drivers/net/ovpn/pktid.c | 130 ++ > drivers/net/ovpn/pktid.h | 87 + > drivers/net/ovpn/proto.h | 104 + > drivers/net/ovpn/skb.h | 56 + > drivers/net/ovpn/socket.c | 178 ++ > drivers/net/ovpn/socket.h | 55 + > drivers/net/ovpn/stats.c | 21 + > drivers/net/ovpn/stats.h | 47 + > drivers/net/ovpn/tcp.c | 506 +++++ > drivers/net/ovpn/tcp.h | 44 + > drivers/net/ovpn/udp.c | 406 ++++ > drivers/net/ovpn/udp.h | 26 + > include/net/netlink.h | 1 + > include/uapi/linux/if_link.h | 15 + > include/uapi/linux/ovpn.h | 109 + > include/uapi/linux/udp.h | 1 + > tools/net/ynl/ynl-gen-c.py | 4 +- > 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 | 183 ++ > tools/testing/selftests/net/ovpn/udp_peers.txt | 5 + > 52 files changed, 9494 insertions(+), 1 deletion(-) > --- > base-commit: ab101c553bc1f76a839163d1dc0d1e715ad6bb4e > change-id: 20241002-b4-ovpn-eeee35c694a2 > > Best regards,
2024-10-29, 11:47:22 +0100, Antonio Quartulli wrote: > +static int ovpn_udp_encap_recv(struct sock *sk, struct sk_buff *skb) > +{ [...] > + opcode = ovpn_opcode_from_skb(skb, sizeof(struct udphdr)); > + if (unlikely(opcode != OVPN_DATA_V2)) { > + /* DATA_V1 is not supported */ > + if (opcode == OVPN_DATA_V1) The TCP encap code passes everything that's not V2 to userspace. Why not do that with UDP as well? > + goto drop; > + > + /* unknown or control packet: let it bubble up to userspace */ > + return 1; > + } > + > + peer_id = ovpn_peer_id_from_skb(skb, sizeof(struct udphdr)); > + /* some OpenVPN server implementations send data packets with the > + * peer-id set to undef. In this case we skip the peer lookup by peer-id > + * and we try with the transport address > + */ > + if (peer_id != OVPN_PEER_ID_UNDEF) { > + peer = ovpn_peer_get_by_id(ovpn, peer_id); > + if (!peer) { > + net_err_ratelimited("%s: received data from unknown peer (id: %d)\n", > + __func__, peer_id); > + goto drop; > + } > + } > + > + if (!peer) { nit: that could be an "else" combined with the previous case? > + /* data packet with undef peer-id */ > + peer = ovpn_peer_get_by_transp_addr(ovpn, skb); > + if (unlikely(!peer)) { > + net_dbg_ratelimited("%s: received data with undef peer-id from unknown source\n", > + __func__); > + goto drop; > + } > + }
Notable changes from v10: * extended commit message of 23/23 with brief description of the output * Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0-b87530777be7@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. 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 (23): netlink: add NLA_POLICY_MAX_LEN macro 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 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 | 362 +++ MAINTAINERS | 11 + drivers/net/Kconfig | 14 + drivers/net/Makefile | 1 + drivers/net/ovpn/Makefile | 22 + drivers/net/ovpn/bind.c | 54 + drivers/net/ovpn/bind.h | 117 + drivers/net/ovpn/crypto.c | 214 ++ drivers/net/ovpn/crypto.h | 145 ++ drivers/net/ovpn/crypto_aead.c | 386 ++++ drivers/net/ovpn/crypto_aead.h | 33 + drivers/net/ovpn/io.c | 462 ++++ drivers/net/ovpn/io.h | 25 + drivers/net/ovpn/main.c | 337 +++ drivers/net/ovpn/main.h | 24 + drivers/net/ovpn/netlink-gen.c | 212 ++ drivers/net/ovpn/netlink-gen.h | 41 + drivers/net/ovpn/netlink.c | 1135 ++++++++++ drivers/net/ovpn/netlink.h | 18 + drivers/net/ovpn/ovpnstruct.h | 61 + drivers/net/ovpn/packet.h | 40 + drivers/net/ovpn/peer.c | 1201 ++++++++++ drivers/net/ovpn/peer.h | 165 ++ drivers/net/ovpn/pktid.c | 130 ++ drivers/net/ovpn/pktid.h | 87 + drivers/net/ovpn/proto.h | 104 + drivers/net/ovpn/skb.h | 56 + drivers/net/ovpn/socket.c | 178 ++ drivers/net/ovpn/socket.h | 55 + drivers/net/ovpn/stats.c | 21 + drivers/net/ovpn/stats.h | 47 + drivers/net/ovpn/tcp.c | 506 +++++ drivers/net/ovpn/tcp.h | 44 + drivers/net/ovpn/udp.c | 406 ++++ drivers/net/ovpn/udp.h | 26 + include/net/netlink.h | 1 + include/uapi/linux/if_link.h | 15 + include/uapi/linux/ovpn.h | 109 + include/uapi/linux/udp.h | 1 + tools/net/ynl/ynl-gen-c.py | 4 +- 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 | 183 ++ tools/testing/selftests/net/ovpn/udp_peers.txt | 5 + 52 files changed, 9494 insertions(+), 1 deletion(-) --- base-commit: ab101c553bc1f76a839163d1dc0d1e715ad6bb4e change-id: 20241002-b4-ovpn-eeee35c694a2 Best regards,