| Message ID | 20241029-b4-ovpn-v11-6-de4698c73a25@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| 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; >> +} >
2024-10-30, 21:47:58 +0100, Antonio Quartulli wrote: > 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. My worry is that code changes over time, assumptions are forgotten, and we end up with code that was a bit odd but safe not being safe anymore. > > > > > + 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". Yeah, true, let's leave it. Thanks.
On 29.10.2024 12:47, Antonio Quartulli wrote: > An ovpn_peer object holds the whole status of a remote peer > (regardless whether it is a server or a client). > > This includes status for crypto, tx/rx buffers, napi, etc. > > Only support for one peer is introduced (P2P mode). > Multi peer support is introduced with a later patch. Reviewing the peer creation/destroying code I came to a generic question. Did you consider keeping a single P2P peer in the peers table as well? Looks like such approach can greatly simply the code by dropping all these 'switch (ovpn->mode)' checks and implementing a unified peer management. The 'peer' field in the main private data structure can be kept to accelerate lookups, still using peers table for management tasks like removing all the peers on the interface teardown. > Along with the ovpn_peer, also the ovpn_bind object is introcued > as the two are strictly related. > An ovpn_bind object wraps a sockaddr representing the local > coordinates being used to talk to a specific peer. > > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > --- > drivers/net/ovpn/Makefile | 2 + > drivers/net/ovpn/bind.c | 58 +++++++ > drivers/net/ovpn/bind.h | 117 ++++++++++++++ Why do we need these bind.c/bind.h files? They contains a minimum of code and still anyway references the peer object. Can we merge these definitions and code into peer.c/peer.h? > drivers/net/ovpn/main.c | 11 ++ > drivers/net/ovpn/main.h | 2 + > drivers/net/ovpn/ovpnstruct.h | 4 + > drivers/net/ovpn/peer.c | 354 ++++++++++++++++++++++++++++++++++++++++++ > drivers/net/ovpn/peer.h | 79 ++++++++++ > 8 files changed, 627 insertions(+) [...] > diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c > new file mode 100644 > index 0000000000000000000000000000000000000000..b4d2ccec2ceddf43bc445b489cc62a578ef0ad0a > --- /dev/null > +++ b/drivers/net/ovpn/bind.c > @@ -0,0 +1,58 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* OpenVPN data channel offload > + * > + * Copyright (C) 2012-2024 OpenVPN, Inc. > + * > + * Author: James Yonan <james@openvpn.net> > + * Antonio Quartulli <antonio@openvpn.net> > + */ > + > +#include <linux/netdevice.h> > +#include <linux/socket.h> > + > +#include "ovpnstruct.h" > +#include "bind.h" > +#include "peer.h" > + > +/** > + * ovpn_bind_from_sockaddr - retrieve binding matching sockaddr > + * @ss: the sockaddr to match > + * > + * Return: the bind matching the passed sockaddr if found, NULL otherwise The function returns ERR_PTR() in case of error, the comment should be updated. > + */ > +struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *ss) > +{ > + struct ovpn_bind *bind; > + size_t sa_len; > + > + if (ss->ss_family == AF_INET) > + sa_len = sizeof(struct sockaddr_in); > + else if (ss->ss_family == AF_INET6) > + sa_len = sizeof(struct sockaddr_in6); > + else > + return ERR_PTR(-EAFNOSUPPORT); > + > + bind = kzalloc(sizeof(*bind), GFP_ATOMIC); > + if (unlikely(!bind)) > + return ERR_PTR(-ENOMEM); > + > + memcpy(&bind->remote, ss, sa_len); > + > + return bind; > +} > + > +/** > + * ovpn_bind_reset - assign new binding to peer > + * @peer: the peer whose binding has to be replaced > + * @new: the new bind to assign > + */ > +void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *new) > +{ > + struct ovpn_bind *old; > + > + spin_lock_bh(&peer->lock); > + old = rcu_replace_pointer(peer->bind, new, true); > + spin_unlock_bh(&peer->lock); Locking will be removed from this function in the subsequent patch. Should we move the peer->lock usage to ovpn_peer_release() now? > + > + kfree_rcu(old, rcu); > +} > diff --git a/drivers/net/ovpn/bind.h b/drivers/net/ovpn/bind.h > new file mode 100644 > index 0000000000000000000000000000000000000000..859213d5040deb36c416eafcf5c6ab31c4d52c7a > --- /dev/null > +++ b/drivers/net/ovpn/bind.h > @@ -0,0 +1,117 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* OpenVPN data channel offload > + * > + * Copyright (C) 2012-2024 OpenVPN, Inc. > + * > + * Author: James Yonan <james@openvpn.net> > + * Antonio Quartulli <antonio@openvpn.net> > + */ > + > +#ifndef _NET_OVPN_OVPNBIND_H_ > +#define _NET_OVPN_OVPNBIND_H_ > + > +#include <net/ip.h> > +#include <linux/in.h> > +#include <linux/in6.h> > +#include <linux/rcupdate.h> > +#include <linux/skbuff.h> > +#include <linux/spinlock.h> > + > +struct ovpn_peer; > + > +/** > + * union ovpn_sockaddr - basic transport layer address Why do we need this dedicated named union? Can we merge this union into the ovpn_bind struct as already done for the local address? > + * @in4: IPv4 address > + * @in6: IPv6 address > + */ > +union ovpn_sockaddr { Family type can be putted here as a dedicated element to make address type check simple: unsigned short int sa_family; > + struct sockaddr_in in4; > + struct sockaddr_in6 in6; > +};> + > +/** > + * struct ovpn_bind - remote peer binding > + * @remote: the remote peer sockaddress > + * @local: local endpoint used to talk to the peer > + * @local.ipv4: local IPv4 used to talk to the peer > + * @local.ipv6: local IPv6 used to talk to the peer > + * @rcu: used to schedule RCU cleanup job > + */ > +struct ovpn_bind { > + union ovpn_sockaddr remote; /* remote sockaddr */ > + > + union { > + struct in_addr ipv4; > + struct in6_addr ipv6; > + } local; > + > + struct rcu_head rcu; > +}; > + > +/** > + * skb_protocol_to_family - translate skb->protocol to AF_INET or AF_INET6 > + * @skb: the packet sk_buff to inspect > + * > + * Return: AF_INET, AF_INET6 or 0 in case of unknown protocol > + */ > +static inline unsigned short skb_protocol_to_family(const struct sk_buff *skb) The function called outside the peer.c file only in ovpn_decrypt_post() and that call is debatable. Considering this, I belive, the skb_protocol_to_family() should be moved inside peer.c as static non-inlined function. > +{ > + switch (skb->protocol) { > + case htons(ETH_P_IP): > + return AF_INET; > + case htons(ETH_P_IPV6): > + return AF_INET6; > + default: > + return 0; > + } > +} > + > +/** > + * ovpn_bind_skb_src_match - match packet source with binding > + * @bind: the binding to match > + * @skb: the packet to match > + * > + * Return: true if the packet source matches the remote peer sockaddr > + * in the binding > + */ > +static inline bool ovpn_bind_skb_src_match(const struct ovpn_bind *bind, > + const struct sk_buff *skb) The function is called only from ovpn_peer_float() and probably should be moved into peer.c and un-inlined. > +{ > + const unsigned short family = skb_protocol_to_family(skb); > + const union ovpn_sockaddr *remote; > + > + if (unlikely(!bind)) > + return false; The caller ovpn_peer_float() function already verified bind object pointer, why we should redo the same check here? > + > + remote = &bind->remote; > + > + if (unlikely(remote->in4.sin_family != family)) > + return false; > + > + switch (family) { > + case AF_INET: > + if (unlikely(remote->in4.sin_addr.s_addr != ip_hdr(skb)->saddr)) > + return false; > + > + if (unlikely(remote->in4.sin_port != udp_hdr(skb)->source)) > + return false; > + break; > + case AF_INET6: > + if (unlikely(!ipv6_addr_equal(&remote->in6.sin6_addr, > + &ipv6_hdr(skb)->saddr))) > + return false; > + > + if (unlikely(remote->in6.sin6_port != udp_hdr(skb)->source)) > + return false; > + break; > + default: > + return false; > + } > + > + return true; > +} > + > +struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *sa); > +void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *bind); > + > +#endif /* _NET_OVPN_OVPNBIND_H_ */ > diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c > index eaa83a8662e4ac2c758201008268f9633643c0b6..5492ce07751d135c1484fe1ed8227c646df94969 100644 > --- a/drivers/net/ovpn/main.c > +++ b/drivers/net/ovpn/main.c > @@ -20,6 +20,7 @@ > #include "netlink.h" > #include "io.h" > #include "packet.h" > +#include "peer.h" > > /* Driver info */ > #define DRV_DESCRIPTION "OpenVPN data channel offload (ovpn)" > @@ -29,6 +30,11 @@ static void ovpn_struct_free(struct net_device *net) > { > } > > +static int ovpn_net_init(struct net_device *dev) > +{ > + return 0; > +} The function is not required. Can we move its introduction to 'implement basic RX path (UDP)' patch, where its content will be added and where counterpart ovpn_net_uninit() function will be introduced? > + > static int ovpn_net_open(struct net_device *dev) > { > /* ovpn keeps the carrier always on to avoid losing IP or route > @@ -49,6 +55,7 @@ static int ovpn_net_stop(struct net_device *dev) > } > > static const struct net_device_ops ovpn_netdev_ops = { > + .ndo_init = ovpn_net_init, > .ndo_open = ovpn_net_open, > .ndo_stop = ovpn_net_stop, > .ndo_start_xmit = ovpn_net_xmit, > @@ -128,6 +135,7 @@ static int ovpn_newlink(struct net *src_net, struct net_device *dev, > > ovpn->dev = dev; > ovpn->mode = mode; > + spin_lock_init(&ovpn->lock); > > /* turn carrier explicitly off after registration, this way state is > * clearly defined > @@ -176,6 +184,9 @@ static int ovpn_netdev_notifier_call(struct notifier_block *nb, > > netif_carrier_off(dev); > ovpn->registered = false; > + > + if (ovpn->mode == OVPN_MODE_P2P) > + ovpn_peer_release_p2p(ovpn); > break; > case NETDEV_POST_INIT: > case NETDEV_GOING_DOWN: > diff --git a/drivers/net/ovpn/main.h b/drivers/net/ovpn/main.h > index 0740a05070a817e0daea7b63a1f4fcebd274eb37..28e5c44816e110974333a7a6a9cf18bd15ae84e6 100644 > --- a/drivers/net/ovpn/main.h > +++ b/drivers/net/ovpn/main.h > @@ -19,4 +19,6 @@ bool ovpn_dev_is_valid(const struct net_device *dev); > #define OVPN_HEAD_ROOM ALIGN(16 + SKB_HEADER_LEN, 4) > #define OVPN_MAX_PADDING 16 > > +#define OVPN_QUEUE_LEN 1024 This macro is unused, should we drop it? > + > #endif /* _NET_OVPN_MAIN_H_ */ > diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ovpnstruct.h > index 211df871538d34fdff90d182f21a0b0fb11b28ad..a22c5083381c131db01a28c0f51e661d690d4998 100644 > --- a/drivers/net/ovpn/ovpnstruct.h > +++ b/drivers/net/ovpn/ovpnstruct.h > @@ -20,6 +20,8 @@ > * @dev_tracker: reference tracker for associated dev > * @registered: whether dev is still registered with netdev or not > * @mode: device operation mode (i.e. p2p, mp, ..) > + * @lock: protect this object > + * @peer: in P2P mode, this is the only remote peer > * @dev_list: entry for the module wide device list > */ > struct ovpn_struct { > @@ -27,6 +29,8 @@ struct ovpn_struct { > netdevice_tracker dev_tracker; > bool registered; > enum ovpn_mode mode; > + spinlock_t lock; /* protect writing to the ovpn_struct object */ > + struct ovpn_peer __rcu *peer; > struct list_head dev_list; > }; > > diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c > new file mode 100644 > index 0000000000000000000000000000000000000000..d9788a0cc99b5839c466c35d1b2266cc6b95fb72 > --- /dev/null > +++ b/drivers/net/ovpn/peer.c > @@ -0,0 +1,354 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* OpenVPN data channel offload > + * > + * Copyright (C) 2020-2024 OpenVPN, Inc. > + * > + * Author: James Yonan <james@openvpn.net> > + * Antonio Quartulli <antonio@openvpn.net> > + */ > + > +#include <linux/skbuff.h> > +#include <linux/list.h> > + > +#include "ovpnstruct.h" > +#include "bind.h" > +#include "io.h" > +#include "main.h" > +#include "netlink.h" > +#include "peer.h" > + > +/** > + * ovpn_peer_new - allocate and initialize a new peer object > + * @ovpn: the openvpn instance inside which the peer should be created > + * @id: the ID assigned to this peer > + * > + * Return: a pointer to the new peer on success or an error code otherwise > + */ > +struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id) > +{ > + struct ovpn_peer *peer; > + int ret; > + > + /* alloc and init peer object */ > + peer = kzalloc(sizeof(*peer), GFP_KERNEL); > + if (!peer) > + return ERR_PTR(-ENOMEM); > + > + peer->id = id; > + peer->halt = false; > + peer->ovpn = ovpn; > + > + peer->vpn_addrs.ipv4.s_addr = htonl(INADDR_ANY); > + peer->vpn_addrs.ipv6 = in6addr_any; > + > + RCU_INIT_POINTER(peer->bind, NULL); > + spin_lock_init(&peer->lock); > + kref_init(&peer->refcount); > + > + ret = dst_cache_init(&peer->dst_cache, GFP_KERNEL); > + if (ret < 0) { > + netdev_err(ovpn->dev, "%s: cannot initialize dst cache\n", > + __func__); > + kfree(peer); > + return ERR_PTR(ret); > + } > + > + netdev_hold(ovpn->dev, &ovpn->dev_tracker, GFP_KERNEL); > + > + return peer; > +} > + > +/** > + * ovpn_peer_release - release peer private members > + * @peer: the peer to release > + */ > +static void ovpn_peer_release(struct ovpn_peer *peer) > +{ > + ovpn_bind_reset(peer, NULL); > + > + dst_cache_destroy(&peer->dst_cache); > + netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker); > + kfree_rcu(peer, rcu); > +} > + > +/** > + * ovpn_peer_release_kref - callback for kref_put > + * @kref: the kref object belonging to the peer > + */ > +void ovpn_peer_release_kref(struct kref *kref) > +{ > + struct ovpn_peer *peer = container_of(kref, struct ovpn_peer, refcount); > + > + ovpn_peer_release(peer); > +} > + > +/** > + * ovpn_peer_skb_to_sockaddr - fill sockaddr with skb source address > + * @skb: the packet to extract data from > + * @ss: the sockaddr to fill > + * > + * Return: true on success or false otherwise > + */ > +static bool ovpn_peer_skb_to_sockaddr(struct sk_buff *skb, > + struct sockaddr_storage *ss) > +{ > + struct sockaddr_in6 *sa6; > + struct sockaddr_in *sa4; > + > + ss->ss_family = skb_protocol_to_family(skb); Why do we need skb_protocol_to_family() call? Can we directly use skb->protocol and ETH_P_IP/ETH_P_IPV6 in the switch? > + switch (ss->ss_family) { > + case AF_INET: > + sa4 = (struct sockaddr_in *)ss; > + sa4->sin_family = AF_INET; > + sa4->sin_addr.s_addr = ip_hdr(skb)->saddr; > + sa4->sin_port = udp_hdr(skb)->source; > + break; > + case AF_INET6: > + sa6 = (struct sockaddr_in6 *)ss; > + sa6->sin6_family = AF_INET6; > + sa6->sin6_addr = ipv6_hdr(skb)->saddr; > + sa6->sin6_port = udp_hdr(skb)->source; > + break; > + default: > + return false; > + } > + > + return true; > +} > + > +/** > + * ovpn_peer_transp_match - check if sockaddr and peer binding match > + * @peer: the peer to get the binding from > + * @ss: the sockaddr to match > + * > + * Return: true if sockaddr and binding match or false otherwise > + */ > +static bool ovpn_peer_transp_match(const struct ovpn_peer *peer, > + const struct sockaddr_storage *ss) > +{ > + struct ovpn_bind *bind = rcu_dereference(peer->bind); > + struct sockaddr_in6 *sa6; > + struct sockaddr_in *sa4; > + > + if (unlikely(!bind)) > + return false; > + > + if (ss->ss_family != bind->remote.in4.sin_family) nit: if the dedicated 'sa_family' element is added into the union, the check can be more straighforward (without 'in4' access): if (ss->ss_family != bind->remote.sa_family) > + return false; > + > + switch (ss->ss_family) { > + case AF_INET: > + sa4 = (struct sockaddr_in *)ss; > + if (sa4->sin_addr.s_addr != bind->remote.in4.sin_addr.s_addr) > + return false; > + if (sa4->sin_port != bind->remote.in4.sin_port) > + return false; > + break; > + case AF_INET6: > + sa6 = (struct sockaddr_in6 *)ss; > + if (!ipv6_addr_equal(&sa6->sin6_addr, > + &bind->remote.in6.sin6_addr)) > + return false; > + if (sa6->sin6_port != bind->remote.in6.sin6_port) > + return false; > + break; > + default: > + return false; > + } > + > + return true; > +} > + > +/** > + * ovpn_peer_get_by_transp_addr_p2p - get peer by transport address in a P2P > + * instance > + * @ovpn: the openvpn instance to search > + * @ss: the transport socket address > + * > + * Return: the peer if found or NULL otherwise > + */ > +static struct ovpn_peer * > +ovpn_peer_get_by_transp_addr_p2p(struct ovpn_struct *ovpn, > + struct sockaddr_storage *ss) > +{ > + struct ovpn_peer *tmp, *peer = NULL; > + > + rcu_read_lock(); > + tmp = rcu_dereference(ovpn->peer); > + if (likely(tmp && ovpn_peer_transp_match(tmp, ss) && > + ovpn_peer_hold(tmp))) > + peer = tmp; > + rcu_read_unlock(); > + > + return peer; > +} > + > +/** > + * ovpn_peer_get_by_transp_addr - retrieve peer by transport address > + * @ovpn: the openvpn instance to search > + * @skb: the skb to retrieve the source transport address from > + * > + * Return: a pointer to the peer if found or NULL otherwise > + */ > +struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct *ovpn, > + struct sk_buff *skb) > +{ > + struct ovpn_peer *peer = NULL; > + struct sockaddr_storage ss = { 0 }; nit: reverse x-mas tree order, please. > + > + if (unlikely(!ovpn_peer_skb_to_sockaddr(skb, &ss))) > + return NULL; > + > + if (ovpn->mode == OVPN_MODE_P2P) > + peer = ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss); > + > + return peer; > +} > + > +/** > + * ovpn_peer_get_by_id_p2p - get peer by ID in a P2P instance > + * @ovpn: the openvpn instance to search > + * @peer_id: the ID of the peer to find > + * > + * Return: the peer if found or NULL otherwise > + */ > +static struct ovpn_peer *ovpn_peer_get_by_id_p2p(struct ovpn_struct *ovpn, > + u32 peer_id) > +{ > + struct ovpn_peer *tmp, *peer = NULL; > + > + rcu_read_lock(); > + tmp = rcu_dereference(ovpn->peer); > + if (likely(tmp && tmp->id == peer_id && ovpn_peer_hold(tmp))) > + peer = tmp; > + rcu_read_unlock(); > + > + return peer; > +} > + > +/** > + * ovpn_peer_get_by_id - retrieve peer by ID > + * @ovpn: the openvpn instance to search > + * @peer_id: the unique peer identifier to match > + * > + * Return: a pointer to the peer if found or NULL otherwise > + */ > +struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id) > +{ > + struct ovpn_peer *peer = NULL; > + > + if (ovpn->mode == OVPN_MODE_P2P) > + peer = ovpn_peer_get_by_id_p2p(ovpn, peer_id); > + > + return peer; > +} > + > +/** > + * ovpn_peer_add_p2p - add peer to related tables in a P2P instance > + * @ovpn: the instance to add the peer to > + * @peer: the peer to add > + * > + * Return: 0 on success or a negative error code otherwise > + */ > +static int ovpn_peer_add_p2p(struct ovpn_struct *ovpn, struct ovpn_peer *peer) > +{ > + struct ovpn_peer *tmp; > + > + spin_lock_bh(&ovpn->lock); > + /* in p2p mode it is possible to have a single peer only, therefore the > + * old one is released and substituted by the new one > + */ > + tmp = rcu_dereference_protected(ovpn->peer, > + lockdep_is_held(&ovpn->lock)); > + if (tmp) { > + tmp->delete_reason = OVPN_DEL_PEER_REASON_TEARDOWN; > + ovpn_peer_put(tmp); > + } > + > + rcu_assign_pointer(ovpn->peer, peer); nit: the rcu_dereference_protected() + rcu_assign_pointer() calls can be replaced with a single rcu_replace_pointer() call. > + spin_unlock_bh(&ovpn->lock); > + > + return 0; > +} > + > +/** > + * ovpn_peer_add - add peer to the related tables > + * @ovpn: the openvpn instance the peer belongs to > + * @peer: the peer object to add > + * > + * Assume refcounter was increased by caller > + * > + * Return: 0 on success or a negative error code otherwise > + */ > +int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) > +{ > + switch (ovpn->mode) { > + case OVPN_MODE_P2P: > + return ovpn_peer_add_p2p(ovpn, peer); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +/** > + * ovpn_peer_del_p2p - delete peer from related tables in a P2P instance > + * @peer: the peer to delete > + * @reason: reason why the peer was deleted (sent to userspace) > + * > + * Return: 0 on success or a negative error code otherwise > + */ > +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); nit: the above two lines can be simplified to: if (DEBUG_NET_WARN_ON_ONCE(tmp != peer)) { > + if (tmp) > + ovpn_peer_put(tmp); > + > + return -ENOENT; > + } > + > + tmp->delete_reason = reason; > + RCU_INIT_POINTER(peer->ovpn->peer, NULL); > + ovpn_peer_put(tmp); > + > + return 0; > +} > + > +/** > + * ovpn_peer_release_p2p - release peer upon P2P device teardown > + * @ovpn: the instance being torn down > + */ > +void ovpn_peer_release_p2p(struct ovpn_struct *ovpn) > +{ > + struct ovpn_peer *tmp; > + > + spin_lock_bh(&ovpn->lock); > + tmp = rcu_dereference_protected(ovpn->peer, > + lockdep_is_held(&ovpn->lock)); > + if (tmp) > + ovpn_peer_del_p2p(tmp, OVPN_DEL_PEER_REASON_TEARDOWN); > + spin_unlock_bh(&ovpn->lock); > +} > + > +/** > + * ovpn_peer_del - delete peer from related tables > + * @peer: the peer object to delete > + * @reason: reason for deleting peer (will be sent to userspace) > + * > + * Return: 0 on success or a negative error code otherwise > + */ > +int ovpn_peer_del(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason) > +{ > + switch (peer->ovpn->mode) { > + case OVPN_MODE_P2P: > + return ovpn_peer_del_p2p(peer, reason); > + default: > + return -EOPNOTSUPP; > + } > +} > diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h > new file mode 100644 > index 0000000000000000000000000000000000000000..6e0c6b14559de886d0677117f5a7ae029214e1f8 > --- /dev/null > +++ b/drivers/net/ovpn/peer.h > @@ -0,0 +1,79 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* OpenVPN data channel offload > + * > + * Copyright (C) 2020-2024 OpenVPN, Inc. > + * > + * Author: James Yonan <james@openvpn.net> > + * Antonio Quartulli <antonio@openvpn.net> > + */ > + > +#ifndef _NET_OVPN_OVPNPEER_H_ > +#define _NET_OVPN_OVPNPEER_H_ > + > +#include <net/dst_cache.h> > + > +/** > + * struct ovpn_peer - the main remote peer object > + * @ovpn: main openvpn instance this peer belongs to > + * @id: unique identifier > + * @vpn_addrs: IP addresses assigned over the tunnel > + * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel > + * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel > + * @dst_cache: cache for dst_entry used to send to peer > + * @bind: remote peer binding > + * @halt: true if ovpn_peer_mark_delete was called > + * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..) > + * @lock: protects binding to peer (bind) > + * @refcount: reference counter > + * @rcu: used to free peer in an RCU safe way > + * @delete_work: deferred cleanup work, used to notify userspace > + */ > +struct ovpn_peer { > + struct ovpn_struct *ovpn; > + u32 id; > + struct { > + struct in_addr ipv4; > + struct in6_addr ipv6; > + } vpn_addrs; > + struct dst_cache dst_cache; > + struct ovpn_bind __rcu *bind; > + bool halt; > + enum ovpn_del_peer_reason delete_reason; > + spinlock_t lock; /* protects bind */ > + struct kref refcount; > + struct rcu_head rcu; > + struct work_struct delete_work; > +}; > + > +/** > + * ovpn_peer_hold - increase reference counter > + * @peer: the peer whose counter should be increased > + * > + * Return: true if the counter was increased or false if it was zero already > + */ > +static inline bool ovpn_peer_hold(struct ovpn_peer *peer) > +{ > + return kref_get_unless_zero(&peer->refcount); > +} > + > +void ovpn_peer_release_kref(struct kref *kref); > + > +/** > + * ovpn_peer_put - decrease reference counter > + * @peer: the peer whose counter should be decreased > + */ > +static inline void ovpn_peer_put(struct ovpn_peer *peer) > +{ > + kref_put(&peer->refcount, ovpn_peer_release_kref); > +} > + > +struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id); > +int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer); > +int ovpn_peer_del(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason); > +void ovpn_peer_release_p2p(struct ovpn_struct *ovpn); > + > +struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct *ovpn, > + struct sk_buff *skb); > +struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id); > + > +#endif /* _NET_OVPN_OVPNPEER_H_ */ >
On 29.10.2024 12:47, Antonio Quartulli wrote: [...] > +static void ovpn_peer_release(struct ovpn_peer *peer) > +{ > + ovpn_bind_reset(peer, NULL); > + nit: this empty line after ovpn_bind_reset() is removed in the 'implement basic TX path (UDP)' patch. What tricks git and it produces a sensless diff with 'ovpn_bind_reset(...)' line beeing removed and then introduced again. If you do not like this empty line then remove it here, please :) > + dst_cache_destroy(&peer->dst_cache); > + netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker); > + kfree_rcu(peer, rcu); > +} -- Sergey
On 05/11/2024 14:12, Sabrina Dubroca wrote: > 2024-10-30, 21:47:58 +0100, Antonio Quartulli wrote: >> 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. > > My worry is that code changes over time, assumptions are forgotten, > and we end up with code that was a bit odd but safe not being safe > anymore. Yeah, makes sense. I'll move the call to dst_cache_destroy() and to kfree(peer) in a RCU callback. Thanks! > >>> >>>> + 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". > > Yeah, true, let's leave it. Thanks. >
2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote: > On 29.10.2024 12:47, Antonio Quartulli wrote: > > An ovpn_peer object holds the whole status of a remote peer > > (regardless whether it is a server or a client). > > > > This includes status for crypto, tx/rx buffers, napi, etc. > > > > Only support for one peer is introduced (P2P mode). > > Multi peer support is introduced with a later patch. > > Reviewing the peer creation/destroying code I came to a generic question. > Did you consider keeping a single P2P peer in the peers table as well? > > Looks like such approach can greatly simply the code by dropping all these > 'switch (ovpn->mode)' checks and implementing a unified peer management. The > 'peer' field in the main private data structure can be kept to accelerate > lookups, still using peers table for management tasks like removing all the > peers on the interface teardown. It would save a few 'switch(mode)', but force every client to allocate the hashtable for no reason at all. That tradeoff doesn't look very beneficial to me, the P2P-specific code is really simple. And if you keep ovpn->peer to make lookups faster, you're not removing that many 'switch(mode)'.
On 12.11.2024 19:31, Sabrina Dubroca wrote: > 2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote: >> On 29.10.2024 12:47, Antonio Quartulli wrote: >>> An ovpn_peer object holds the whole status of a remote peer >>> (regardless whether it is a server or a client). >>> >>> This includes status for crypto, tx/rx buffers, napi, etc. >>> >>> Only support for one peer is introduced (P2P mode). >>> Multi peer support is introduced with a later patch. >> >> Reviewing the peer creation/destroying code I came to a generic question. >> Did you consider keeping a single P2P peer in the peers table as well? >> >> Looks like such approach can greatly simply the code by dropping all these >> 'switch (ovpn->mode)' checks and implementing a unified peer management. The >> 'peer' field in the main private data structure can be kept to accelerate >> lookups, still using peers table for management tasks like removing all the >> peers on the interface teardown. > > It would save a few 'switch(mode)', but force every client to allocate > the hashtable for no reason at all. That tradeoff doesn't look very > beneficial to me, the P2P-specific code is really simple. And if you > keep ovpn->peer to make lookups faster, you're not removing that many > 'switch(mode)'. Looking at the done review, I can retrospectively conclude that I personally do not like short 'switch' statements and special handlers :) Seriously, this module has a highest density of switches per KLOC from what I have seen before and a major part of it dedicated to handle the special case of P2P connection. What together look too unusual, so it feels like a flaw in the design. I racked my brains to come up with a better solution and failed. So I took a different approach, inviting people to discuss item pieces of the code to find a solution collectively or to realize that there is no better solution for now. The problem is that all these hash tables become inefficient with the single entry (P2P case). I was thinking about allocating a table with a single bin, but it still requires hash function run to access the indexed entry. And back to the hashtable(s) size for the MP mode. 8k-bins table looks a good choice for a normal server with 1-2Gb uplink serving up to 1k connections. But it sill unclear, how this choice can affect installations with a bigger number of connections? Or is this module applicable for embedded solutions? E.g. running a couple of VPN servers on a home router with a few actual connections looks like a waste of RAM. I was about to suggest to use rhashtable due to its dynamic sizing feature, but the module needs three tables. Any better idea? -- Sergey
2024-11-13, 03:37:13 +0200, Sergey Ryazanov wrote: > On 12.11.2024 19:31, Sabrina Dubroca wrote: > > 2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote: > > > On 29.10.2024 12:47, Antonio Quartulli wrote: > > > > An ovpn_peer object holds the whole status of a remote peer > > > > (regardless whether it is a server or a client). > > > > > > > > This includes status for crypto, tx/rx buffers, napi, etc. > > > > > > > > Only support for one peer is introduced (P2P mode). > > > > Multi peer support is introduced with a later patch. > > > > > > Reviewing the peer creation/destroying code I came to a generic question. > > > Did you consider keeping a single P2P peer in the peers table as well? > > > > > > Looks like such approach can greatly simply the code by dropping all these > > > 'switch (ovpn->mode)' checks and implementing a unified peer management. The > > > 'peer' field in the main private data structure can be kept to accelerate > > > lookups, still using peers table for management tasks like removing all the > > > peers on the interface teardown. > > > > It would save a few 'switch(mode)', but force every client to allocate > > the hashtable for no reason at all. That tradeoff doesn't look very > > beneficial to me, the P2P-specific code is really simple. And if you > > keep ovpn->peer to make lookups faster, you're not removing that many > > 'switch(mode)'. > > Looking at the done review, I can retrospectively conclude that I personally > do not like short 'switch' statements and special handlers :) > > Seriously, this module has a highest density of switches per KLOC from what > I have seen before and a major part of it dedicated to handle the special > case of P2P connection. I think it's fine. Either way there will be two implementations of whatever mode-dependent operation needs to be done. switch doesn't make it more complex than an ops structure. If you're reading the current version and find ovpn_peer_add, you see directly that it'll do either ovpn_peer_add_mp or ovpn_peer_add_p2p. With an ops structure, you'd have a call to ovpn->ops->peer_add, and you'd have to look up all possible ops structures to know that it can be either ovpn_peer_add_mp or ovpn_peer_add_p2p. If there's an undefined number of implementations living in different modules (like net_device_ops, or L4 protocols), you don't have a choice. xfrm went the opposite way to what you're proposing a few years ago (see commit 0c620e97b349 ("xfrm: remove output indirection from xfrm_mode") and others), and it made the code simpler. > What together look too unusual, so it feels like a > flaw in the design. I don't think it's a flaw in the design, maybe just different needs from other code you've seen (but similar in some ways to xfrm). > I racked my brains to come up with a better solution and > failed. So I took a different approach, inviting people to discuss item > pieces of the code to find a solution collectively or to realize that there > is no better solution for now. Sure. And I think there is no better solution, so I'm answering this thread to say that. > The problem is that all these hash tables become inefficient with the single > entry (P2P case). I was thinking about allocating a table with a single bin, > but it still requires hash function run to access the indexed entry. And the current implementation relies on fixed-size hashtables (hash_for_each_safe -> HASH_SIZE -> ARRAY_SIZE -> sizeof). > And back to the hashtable(s) size for the MP mode. 8k-bins table looks a > good choice for a normal server with 1-2Gb uplink serving up to 1k > connections. But it sill unclear, how this choice can affect installations > with a bigger number of connections? Or is this module applicable for > embedded solutions? E.g. running a couple of VPN servers on a home router > with a few actual connections looks like a waste of RAM. I was about to > suggest to use rhashtable due to its dynamic sizing feature, but the module > needs three tables. Any better idea? For this initial implementation I think it's fine. Sure, converting to rhashtable (or some other type of dynamically-sized hashtable, if rhashtable doesn't fit) in the future would make sense. But I don't think it's necessary to get the patches into net-next.
On 10/11/2024 20:52, Sergey Ryazanov wrote: > On 29.10.2024 12:47, Antonio Quartulli wrote: > > [...] > >> +static void ovpn_peer_release(struct ovpn_peer *peer) >> +{ >> + ovpn_bind_reset(peer, NULL); >> + > > nit: this empty line after ovpn_bind_reset() is removed in the > 'implement basic TX path (UDP)' patch. What tricks git and it produces a > sensless diff with 'ovpn_bind_reset(...)' line beeing removed and then > introduced again. If you do not like this empty line then remove it > here, please :) Thanks! will make sure it won't be introduced at all. Regards, > >> + dst_cache_destroy(&peer->dst_cache); >> + netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker); >> + kfree_rcu(peer, rcu); >> +} > > -- > Sergey
2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote: > +/** > + * struct ovpn_peer - the main remote peer object > + * @ovpn: main openvpn instance this peer belongs to > + * @id: unique identifier > + * @vpn_addrs: IP addresses assigned over the tunnel > + * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel > + * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel > + * @dst_cache: cache for dst_entry used to send to peer > + * @bind: remote peer binding > + * @halt: true if ovpn_peer_mark_delete was called nit: It's initialized to false in ovpn_peer_new, but then never set to true nor read. Drop it? > + * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..) > + * @lock: protects binding to peer (bind) nit: as well as the keepalive values that are introduced later? (I guess the comment should be fixed up in patch 15 when the keepalive mechanism is added)
On 13.11.2024 12:03, Sabrina Dubroca wrote: > 2024-11-13, 03:37:13 +0200, Sergey Ryazanov wrote: >> On 12.11.2024 19:31, Sabrina Dubroca wrote: >>> 2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote: >>>> On 29.10.2024 12:47, Antonio Quartulli wrote: >>>>> An ovpn_peer object holds the whole status of a remote peer >>>>> (regardless whether it is a server or a client). >>>>> >>>>> This includes status for crypto, tx/rx buffers, napi, etc. >>>>> >>>>> Only support for one peer is introduced (P2P mode). >>>>> Multi peer support is introduced with a later patch. >>>> >>>> Reviewing the peer creation/destroying code I came to a generic question. >>>> Did you consider keeping a single P2P peer in the peers table as well? >>>> >>>> Looks like such approach can greatly simply the code by dropping all these >>>> 'switch (ovpn->mode)' checks and implementing a unified peer management. The >>>> 'peer' field in the main private data structure can be kept to accelerate >>>> lookups, still using peers table for management tasks like removing all the >>>> peers on the interface teardown. >>> >>> It would save a few 'switch(mode)', but force every client to allocate >>> the hashtable for no reason at all. That tradeoff doesn't look very >>> beneficial to me, the P2P-specific code is really simple. And if you >>> keep ovpn->peer to make lookups faster, you're not removing that many >>> 'switch(mode)'. >> >> Looking at the done review, I can retrospectively conclude that I personally >> do not like short 'switch' statements and special handlers :) >> >> Seriously, this module has a highest density of switches per KLOC from what >> I have seen before and a major part of it dedicated to handle the special >> case of P2P connection. > > I think it's fine. Either way there will be two implementations of > whatever mode-dependent operation needs to be done. switch doesn't > make it more complex than an ops structure. > > If you're reading the current version and find ovpn_peer_add, you see > directly that it'll do either ovpn_peer_add_mp or > ovpn_peer_add_p2p. With an ops structure, you'd have a call to > ovpn->ops->peer_add, and you'd have to look up all possible ops > structures to know that it can be either ovpn_peer_add_mp or > ovpn_peer_add_p2p. If there's an undefined number of implementations > living in different modules (like net_device_ops, or L4 protocols), > you don't have a choice. > > xfrm went the opposite way to what you're proposing a few years ago > (see commit 0c620e97b349 ("xfrm: remove output indirection from > xfrm_mode") and others), and it made the code simpler. I checked this. Florian did a nice rework. And the way of implementation looks reasonable since there are more than two encapsulation modes and handling is more complex than just selecting a function to call. What I don't like about switches, that it requires extra lines of code and pushes an author to introduce a default case with error handling. It was mentioned that the module unlikely going to support more than two modes. In this context shall we consider ternary operator usage. E.g.: next_run = ovpn->mode == OVPN_MODE_P2P ? ovpn_peer_keepalive_work_p2p(...) : ovpn_peer_keepalive_work_mp(...); >> And back to the hashtable(s) size for the MP mode. 8k-bins table looks a >> good choice for a normal server with 1-2Gb uplink serving up to 1k >> connections. But it sill unclear, how this choice can affect installations >> with a bigger number of connections? Or is this module applicable for >> embedded solutions? E.g. running a couple of VPN servers on a home router >> with a few actual connections looks like a waste of RAM. I was about to >> suggest to use rhashtable due to its dynamic sizing feature, but the module >> needs three tables. Any better idea? > > For this initial implementation I think it's fine. Sure, converting to > rhashtable (or some other type of dynamically-sized hashtable, if > rhashtable doesn't fit) in the future would make sense. But I don't > think it's necessary to get the patches into net-next. Make sense. Thanks for sharing these thoughts. -- Sergey
On 21/11/2024 00:22, Sergey Ryazanov wrote: > On 13.11.2024 12:03, Sabrina Dubroca wrote: >> 2024-11-13, 03:37:13 +0200, Sergey Ryazanov wrote: >>> On 12.11.2024 19:31, Sabrina Dubroca wrote: >>>> 2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote: >>>>> On 29.10.2024 12:47, Antonio Quartulli wrote: >>>>>> An ovpn_peer object holds the whole status of a remote peer >>>>>> (regardless whether it is a server or a client). >>>>>> >>>>>> This includes status for crypto, tx/rx buffers, napi, etc. >>>>>> >>>>>> Only support for one peer is introduced (P2P mode). >>>>>> Multi peer support is introduced with a later patch. >>>>> >>>>> Reviewing the peer creation/destroying code I came to a generic >>>>> question. >>>>> Did you consider keeping a single P2P peer in the peers table as well? >>>>> >>>>> Looks like such approach can greatly simply the code by dropping >>>>> all these >>>>> 'switch (ovpn->mode)' checks and implementing a unified peer >>>>> management. The >>>>> 'peer' field in the main private data structure can be kept to >>>>> accelerate >>>>> lookups, still using peers table for management tasks like removing >>>>> all the >>>>> peers on the interface teardown. >>>> >>>> It would save a few 'switch(mode)', but force every client to allocate >>>> the hashtable for no reason at all. That tradeoff doesn't look very >>>> beneficial to me, the P2P-specific code is really simple. And if you >>>> keep ovpn->peer to make lookups faster, you're not removing that many >>>> 'switch(mode)'. >>> >>> Looking at the done review, I can retrospectively conclude that I >>> personally >>> do not like short 'switch' statements and special handlers :) >>> >>> Seriously, this module has a highest density of switches per KLOC >>> from what >>> I have seen before and a major part of it dedicated to handle the >>> special >>> case of P2P connection. >> >> I think it's fine. Either way there will be two implementations of >> whatever mode-dependent operation needs to be done. switch doesn't >> make it more complex than an ops structure. >> >> If you're reading the current version and find ovpn_peer_add, you see >> directly that it'll do either ovpn_peer_add_mp or >> ovpn_peer_add_p2p. With an ops structure, you'd have a call to >> ovpn->ops->peer_add, and you'd have to look up all possible ops >> structures to know that it can be either ovpn_peer_add_mp or >> ovpn_peer_add_p2p. If there's an undefined number of implementations >> living in different modules (like net_device_ops, or L4 protocols), >> you don't have a choice. >> >> xfrm went the opposite way to what you're proposing a few years ago >> (see commit 0c620e97b349 ("xfrm: remove output indirection from >> xfrm_mode") and others), and it made the code simpler. > > I checked this. Florian did a nice rework. And the way of implementation > looks reasonable since there are more than two encapsulation modes and > handling is more complex than just selecting a function to call. > > What I don't like about switches, that it requires extra lines of code > and pushes an author to introduce a default case with error handling. It > was mentioned that the module unlikely going to support more than two > modes. In this context shall we consider ternary operator usage. E.g.: the default case can actually be dropped. That way we can have the compiler warn when one of the enum values is not handled in the switch (should there be a new one at some point). However, the default is just a sanity check against future code changes which may introduce a bug. > > next_run = ovpn->mode == OVPN_MODE_P2P ? > ovpn_peer_keepalive_work_p2p(...) : > ovpn_peer_keepalive_work_mp(...); I find this ugly to read :-) The switch is much more elegant and straightforward. Do you agree this is getting more into a bike shed coloring discussion? :-D Since there is not much gain in changing approach, I think it is better if the maintainer picks a style that he finds more suitable (or simply likes more). no? > >>> And back to the hashtable(s) size for the MP mode. 8k-bins table looks a >>> good choice for a normal server with 1-2Gb uplink serving up to 1k >>> connections. But it sill unclear, how this choice can affect >>> installations >>> with a bigger number of connections? Or is this module applicable for >>> embedded solutions? E.g. running a couple of VPN servers on a home >>> router >>> with a few actual connections looks like a waste of RAM. I was about to >>> suggest to use rhashtable due to its dynamic sizing feature, but the >>> module >>> needs three tables. Any better idea? >> >> For this initial implementation I think it's fine. Sure, converting to >> rhashtable (or some other type of dynamically-sized hashtable, if >> rhashtable doesn't fit) in the future would make sense. But I don't >> think it's necessary to get the patches into net-next. Agreed. It's in the pipe (along with other features that I have already implemented), but it will come later. Regards, > > Make sense. Thanks for sharing these thoughts. > > -- > Sergey
On 20/11/2024 12:56, Sabrina Dubroca wrote: > 2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote: >> +/** >> + * struct ovpn_peer - the main remote peer object >> + * @ovpn: main openvpn instance this peer belongs to >> + * @id: unique identifier >> + * @vpn_addrs: IP addresses assigned over the tunnel >> + * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel >> + * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel >> + * @dst_cache: cache for dst_entry used to send to peer >> + * @bind: remote peer binding >> + * @halt: true if ovpn_peer_mark_delete was called > > nit: It's initialized to false in ovpn_peer_new, but then never set to > true nor read. Drop it? argh. leftover from some older version. Thanks > >> + * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..) >> + * @lock: protects binding to peer (bind) > > nit: as well as the keepalive values that are introduced later? > (I guess the comment should be fixed up in patch 15 when the keepalive > mechanism is added) ACK >
On 21.11.2024 23:23, Antonio Quartulli wrote: > On 21/11/2024 00:22, Sergey Ryazanov wrote: >> On 13.11.2024 12:03, Sabrina Dubroca wrote: >>> 2024-11-13, 03:37:13 +0200, Sergey Ryazanov wrote: >>>> On 12.11.2024 19:31, Sabrina Dubroca wrote: >>>>> 2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote: >>>>>> On 29.10.2024 12:47, Antonio Quartulli wrote: >>>>>>> An ovpn_peer object holds the whole status of a remote peer >>>>>>> (regardless whether it is a server or a client). >>>>>>> >>>>>>> This includes status for crypto, tx/rx buffers, napi, etc. >>>>>>> >>>>>>> Only support for one peer is introduced (P2P mode). >>>>>>> Multi peer support is introduced with a later patch. >>>>>> >>>>>> Reviewing the peer creation/destroying code I came to a generic >>>>>> question. >>>>>> Did you consider keeping a single P2P peer in the peers table as >>>>>> well? >>>>>> >>>>>> Looks like such approach can greatly simply the code by dropping >>>>>> all these >>>>>> 'switch (ovpn->mode)' checks and implementing a unified peer >>>>>> management. The >>>>>> 'peer' field in the main private data structure can be kept to >>>>>> accelerate >>>>>> lookups, still using peers table for management tasks like >>>>>> removing all the >>>>>> peers on the interface teardown. >>>>> >>>>> It would save a few 'switch(mode)', but force every client to allocate >>>>> the hashtable for no reason at all. That tradeoff doesn't look very >>>>> beneficial to me, the P2P-specific code is really simple. And if you >>>>> keep ovpn->peer to make lookups faster, you're not removing that many >>>>> 'switch(mode)'. >>>> >>>> Looking at the done review, I can retrospectively conclude that I >>>> personally >>>> do not like short 'switch' statements and special handlers :) >>>> >>>> Seriously, this module has a highest density of switches per KLOC >>>> from what >>>> I have seen before and a major part of it dedicated to handle the >>>> special >>>> case of P2P connection. >>> >>> I think it's fine. Either way there will be two implementations of >>> whatever mode-dependent operation needs to be done. switch doesn't >>> make it more complex than an ops structure. >>> >>> If you're reading the current version and find ovpn_peer_add, you see >>> directly that it'll do either ovpn_peer_add_mp or >>> ovpn_peer_add_p2p. With an ops structure, you'd have a call to >>> ovpn->ops->peer_add, and you'd have to look up all possible ops >>> structures to know that it can be either ovpn_peer_add_mp or >>> ovpn_peer_add_p2p. If there's an undefined number of implementations >>> living in different modules (like net_device_ops, or L4 protocols), >>> you don't have a choice. >>> >>> xfrm went the opposite way to what you're proposing a few years ago >>> (see commit 0c620e97b349 ("xfrm: remove output indirection from >>> xfrm_mode") and others), and it made the code simpler. >> >> I checked this. Florian did a nice rework. And the way of >> implementation looks reasonable since there are more than two >> encapsulation modes and handling is more complex than just selecting a >> function to call. >> >> What I don't like about switches, that it requires extra lines of code >> and pushes an author to introduce a default case with error handling. >> It was mentioned that the module unlikely going to support more than >> two modes. In this context shall we consider ternary operator usage. >> E.g.: > > the default case can actually be dropped. That way we can have the > compiler warn when one of the enum values is not handled in the switch > (should there be a new one at some point). > However, the default is just a sanity check against future code changes > which may introduce a bug. > >> >> next_run = ovpn->mode == OVPN_MODE_P2P ? >> ovpn_peer_keepalive_work_p2p(...) : >> ovpn_peer_keepalive_work_mp(...); > > I find this ugly to read :-) Yeah. Doesn't look pretty as well. Just to conclude the discussion. Considering what we discussed here and the Sabrina's point regarding the trampoline penalty for indirect invocation, we do not have a better solution for now other than using switches everywhere. -- Sergey
diff --git a/drivers/net/ovpn/Makefile b/drivers/net/ovpn/Makefile index 201dc001419f1d99ae95c0ee0f96e68f8a4eac16..ce13499b3e1775a7f2a9ce16c6cb0aa088f93685 100644 --- a/drivers/net/ovpn/Makefile +++ b/drivers/net/ovpn/Makefile @@ -7,7 +7,9 @@ # Author: Antonio Quartulli <antonio@openvpn.net> obj-$(CONFIG_OVPN) := ovpn.o +ovpn-y += bind.o ovpn-y += main.o ovpn-y += io.o ovpn-y += netlink.o ovpn-y += netlink-gen.o +ovpn-y += peer.o diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c new file mode 100644 index 0000000000000000000000000000000000000000..b4d2ccec2ceddf43bc445b489cc62a578ef0ad0a --- /dev/null +++ b/drivers/net/ovpn/bind.c @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload + * + * Copyright (C) 2012-2024 OpenVPN, Inc. + * + * Author: James Yonan <james@openvpn.net> + * Antonio Quartulli <antonio@openvpn.net> + */ + +#include <linux/netdevice.h> +#include <linux/socket.h> + +#include "ovpnstruct.h" +#include "bind.h" +#include "peer.h" + +/** + * ovpn_bind_from_sockaddr - retrieve binding matching sockaddr + * @ss: the sockaddr to match + * + * Return: the bind matching the passed sockaddr if found, NULL otherwise + */ +struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *ss) +{ + struct ovpn_bind *bind; + size_t sa_len; + + if (ss->ss_family == AF_INET) + sa_len = sizeof(struct sockaddr_in); + else if (ss->ss_family == AF_INET6) + sa_len = sizeof(struct sockaddr_in6); + else + return ERR_PTR(-EAFNOSUPPORT); + + bind = kzalloc(sizeof(*bind), GFP_ATOMIC); + if (unlikely(!bind)) + return ERR_PTR(-ENOMEM); + + memcpy(&bind->remote, ss, sa_len); + + return bind; +} + +/** + * ovpn_bind_reset - assign new binding to peer + * @peer: the peer whose binding has to be replaced + * @new: the new bind to assign + */ +void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *new) +{ + struct ovpn_bind *old; + + spin_lock_bh(&peer->lock); + old = rcu_replace_pointer(peer->bind, new, true); + spin_unlock_bh(&peer->lock); + + kfree_rcu(old, rcu); +} diff --git a/drivers/net/ovpn/bind.h b/drivers/net/ovpn/bind.h new file mode 100644 index 0000000000000000000000000000000000000000..859213d5040deb36c416eafcf5c6ab31c4d52c7a --- /dev/null +++ b/drivers/net/ovpn/bind.h @@ -0,0 +1,117 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload + * + * Copyright (C) 2012-2024 OpenVPN, Inc. + * + * Author: James Yonan <james@openvpn.net> + * Antonio Quartulli <antonio@openvpn.net> + */ + +#ifndef _NET_OVPN_OVPNBIND_H_ +#define _NET_OVPN_OVPNBIND_H_ + +#include <net/ip.h> +#include <linux/in.h> +#include <linux/in6.h> +#include <linux/rcupdate.h> +#include <linux/skbuff.h> +#include <linux/spinlock.h> + +struct ovpn_peer; + +/** + * union ovpn_sockaddr - basic transport layer address + * @in4: IPv4 address + * @in6: IPv6 address + */ +union ovpn_sockaddr { + struct sockaddr_in in4; + struct sockaddr_in6 in6; +}; + +/** + * struct ovpn_bind - remote peer binding + * @remote: the remote peer sockaddress + * @local: local endpoint used to talk to the peer + * @local.ipv4: local IPv4 used to talk to the peer + * @local.ipv6: local IPv6 used to talk to the peer + * @rcu: used to schedule RCU cleanup job + */ +struct ovpn_bind { + union ovpn_sockaddr remote; /* remote sockaddr */ + + union { + struct in_addr ipv4; + struct in6_addr ipv6; + } local; + + struct rcu_head rcu; +}; + +/** + * skb_protocol_to_family - translate skb->protocol to AF_INET or AF_INET6 + * @skb: the packet sk_buff to inspect + * + * Return: AF_INET, AF_INET6 or 0 in case of unknown protocol + */ +static inline unsigned short skb_protocol_to_family(const struct sk_buff *skb) +{ + switch (skb->protocol) { + case htons(ETH_P_IP): + return AF_INET; + case htons(ETH_P_IPV6): + return AF_INET6; + default: + return 0; + } +} + +/** + * ovpn_bind_skb_src_match - match packet source with binding + * @bind: the binding to match + * @skb: the packet to match + * + * Return: true if the packet source matches the remote peer sockaddr + * in the binding + */ +static inline bool ovpn_bind_skb_src_match(const struct ovpn_bind *bind, + const struct sk_buff *skb) +{ + const unsigned short family = skb_protocol_to_family(skb); + const union ovpn_sockaddr *remote; + + if (unlikely(!bind)) + return false; + + remote = &bind->remote; + + if (unlikely(remote->in4.sin_family != family)) + return false; + + switch (family) { + case AF_INET: + if (unlikely(remote->in4.sin_addr.s_addr != ip_hdr(skb)->saddr)) + return false; + + if (unlikely(remote->in4.sin_port != udp_hdr(skb)->source)) + return false; + break; + case AF_INET6: + if (unlikely(!ipv6_addr_equal(&remote->in6.sin6_addr, + &ipv6_hdr(skb)->saddr))) + return false; + + if (unlikely(remote->in6.sin6_port != udp_hdr(skb)->source)) + return false; + break; + default: + return false; + } + + return true; +} + +struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *sa); +void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *bind); + +#endif /* _NET_OVPN_OVPNBIND_H_ */ diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index eaa83a8662e4ac2c758201008268f9633643c0b6..5492ce07751d135c1484fe1ed8227c646df94969 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c @@ -20,6 +20,7 @@ #include "netlink.h" #include "io.h" #include "packet.h" +#include "peer.h" /* Driver info */ #define DRV_DESCRIPTION "OpenVPN data channel offload (ovpn)" @@ -29,6 +30,11 @@ static void ovpn_struct_free(struct net_device *net) { } +static int ovpn_net_init(struct net_device *dev) +{ + return 0; +} + static int ovpn_net_open(struct net_device *dev) { /* ovpn keeps the carrier always on to avoid losing IP or route @@ -49,6 +55,7 @@ static int ovpn_net_stop(struct net_device *dev) } static const struct net_device_ops ovpn_netdev_ops = { + .ndo_init = ovpn_net_init, .ndo_open = ovpn_net_open, .ndo_stop = ovpn_net_stop, .ndo_start_xmit = ovpn_net_xmit, @@ -128,6 +135,7 @@ static int ovpn_newlink(struct net *src_net, struct net_device *dev, ovpn->dev = dev; ovpn->mode = mode; + spin_lock_init(&ovpn->lock); /* turn carrier explicitly off after registration, this way state is * clearly defined @@ -176,6 +184,9 @@ static int ovpn_netdev_notifier_call(struct notifier_block *nb, netif_carrier_off(dev); ovpn->registered = false; + + if (ovpn->mode == OVPN_MODE_P2P) + ovpn_peer_release_p2p(ovpn); break; case NETDEV_POST_INIT: case NETDEV_GOING_DOWN: diff --git a/drivers/net/ovpn/main.h b/drivers/net/ovpn/main.h index 0740a05070a817e0daea7b63a1f4fcebd274eb37..28e5c44816e110974333a7a6a9cf18bd15ae84e6 100644 --- a/drivers/net/ovpn/main.h +++ b/drivers/net/ovpn/main.h @@ -19,4 +19,6 @@ bool ovpn_dev_is_valid(const struct net_device *dev); #define OVPN_HEAD_ROOM ALIGN(16 + SKB_HEADER_LEN, 4) #define OVPN_MAX_PADDING 16 +#define OVPN_QUEUE_LEN 1024 + #endif /* _NET_OVPN_MAIN_H_ */ diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ovpnstruct.h index 211df871538d34fdff90d182f21a0b0fb11b28ad..a22c5083381c131db01a28c0f51e661d690d4998 100644 --- a/drivers/net/ovpn/ovpnstruct.h +++ b/drivers/net/ovpn/ovpnstruct.h @@ -20,6 +20,8 @@ * @dev_tracker: reference tracker for associated dev * @registered: whether dev is still registered with netdev or not * @mode: device operation mode (i.e. p2p, mp, ..) + * @lock: protect this object + * @peer: in P2P mode, this is the only remote peer * @dev_list: entry for the module wide device list */ struct ovpn_struct { @@ -27,6 +29,8 @@ struct ovpn_struct { netdevice_tracker dev_tracker; bool registered; enum ovpn_mode mode; + spinlock_t lock; /* protect writing to the ovpn_struct object */ + struct ovpn_peer __rcu *peer; struct list_head dev_list; }; diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c new file mode 100644 index 0000000000000000000000000000000000000000..d9788a0cc99b5839c466c35d1b2266cc6b95fb72 --- /dev/null +++ b/drivers/net/ovpn/peer.c @@ -0,0 +1,354 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload + * + * Copyright (C) 2020-2024 OpenVPN, Inc. + * + * Author: James Yonan <james@openvpn.net> + * Antonio Quartulli <antonio@openvpn.net> + */ + +#include <linux/skbuff.h> +#include <linux/list.h> + +#include "ovpnstruct.h" +#include "bind.h" +#include "io.h" +#include "main.h" +#include "netlink.h" +#include "peer.h" + +/** + * ovpn_peer_new - allocate and initialize a new peer object + * @ovpn: the openvpn instance inside which the peer should be created + * @id: the ID assigned to this peer + * + * Return: a pointer to the new peer on success or an error code otherwise + */ +struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id) +{ + struct ovpn_peer *peer; + int ret; + + /* alloc and init peer object */ + peer = kzalloc(sizeof(*peer), GFP_KERNEL); + if (!peer) + return ERR_PTR(-ENOMEM); + + peer->id = id; + peer->halt = false; + peer->ovpn = ovpn; + + peer->vpn_addrs.ipv4.s_addr = htonl(INADDR_ANY); + peer->vpn_addrs.ipv6 = in6addr_any; + + RCU_INIT_POINTER(peer->bind, NULL); + spin_lock_init(&peer->lock); + kref_init(&peer->refcount); + + ret = dst_cache_init(&peer->dst_cache, GFP_KERNEL); + if (ret < 0) { + netdev_err(ovpn->dev, "%s: cannot initialize dst cache\n", + __func__); + kfree(peer); + return ERR_PTR(ret); + } + + netdev_hold(ovpn->dev, &ovpn->dev_tracker, GFP_KERNEL); + + return peer; +} + +/** + * ovpn_peer_release - release peer private members + * @peer: the peer to release + */ +static void ovpn_peer_release(struct ovpn_peer *peer) +{ + ovpn_bind_reset(peer, NULL); + + dst_cache_destroy(&peer->dst_cache); + netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker); + kfree_rcu(peer, rcu); +} + +/** + * ovpn_peer_release_kref - callback for kref_put + * @kref: the kref object belonging to the peer + */ +void ovpn_peer_release_kref(struct kref *kref) +{ + struct ovpn_peer *peer = container_of(kref, struct ovpn_peer, refcount); + + ovpn_peer_release(peer); +} + +/** + * ovpn_peer_skb_to_sockaddr - fill sockaddr with skb source address + * @skb: the packet to extract data from + * @ss: the sockaddr to fill + * + * Return: true on success or false otherwise + */ +static bool ovpn_peer_skb_to_sockaddr(struct sk_buff *skb, + struct sockaddr_storage *ss) +{ + struct sockaddr_in6 *sa6; + struct sockaddr_in *sa4; + + ss->ss_family = skb_protocol_to_family(skb); + switch (ss->ss_family) { + case AF_INET: + sa4 = (struct sockaddr_in *)ss; + sa4->sin_family = AF_INET; + sa4->sin_addr.s_addr = ip_hdr(skb)->saddr; + sa4->sin_port = udp_hdr(skb)->source; + break; + case AF_INET6: + sa6 = (struct sockaddr_in6 *)ss; + sa6->sin6_family = AF_INET6; + sa6->sin6_addr = ipv6_hdr(skb)->saddr; + sa6->sin6_port = udp_hdr(skb)->source; + break; + default: + return false; + } + + return true; +} + +/** + * ovpn_peer_transp_match - check if sockaddr and peer binding match + * @peer: the peer to get the binding from + * @ss: the sockaddr to match + * + * Return: true if sockaddr and binding match or false otherwise + */ +static bool ovpn_peer_transp_match(const struct ovpn_peer *peer, + const struct sockaddr_storage *ss) +{ + struct ovpn_bind *bind = rcu_dereference(peer->bind); + struct sockaddr_in6 *sa6; + struct sockaddr_in *sa4; + + if (unlikely(!bind)) + return false; + + if (ss->ss_family != bind->remote.in4.sin_family) + return false; + + switch (ss->ss_family) { + case AF_INET: + sa4 = (struct sockaddr_in *)ss; + if (sa4->sin_addr.s_addr != bind->remote.in4.sin_addr.s_addr) + return false; + if (sa4->sin_port != bind->remote.in4.sin_port) + return false; + break; + case AF_INET6: + sa6 = (struct sockaddr_in6 *)ss; + if (!ipv6_addr_equal(&sa6->sin6_addr, + &bind->remote.in6.sin6_addr)) + return false; + if (sa6->sin6_port != bind->remote.in6.sin6_port) + return false; + break; + default: + return false; + } + + return true; +} + +/** + * ovpn_peer_get_by_transp_addr_p2p - get peer by transport address in a P2P + * instance + * @ovpn: the openvpn instance to search + * @ss: the transport socket address + * + * Return: the peer if found or NULL otherwise + */ +static struct ovpn_peer * +ovpn_peer_get_by_transp_addr_p2p(struct ovpn_struct *ovpn, + struct sockaddr_storage *ss) +{ + struct ovpn_peer *tmp, *peer = NULL; + + rcu_read_lock(); + tmp = rcu_dereference(ovpn->peer); + if (likely(tmp && ovpn_peer_transp_match(tmp, ss) && + ovpn_peer_hold(tmp))) + peer = tmp; + rcu_read_unlock(); + + return peer; +} + +/** + * ovpn_peer_get_by_transp_addr - retrieve peer by transport address + * @ovpn: the openvpn instance to search + * @skb: the skb to retrieve the source transport address from + * + * Return: a pointer to the peer if found or NULL otherwise + */ +struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct *ovpn, + struct sk_buff *skb) +{ + struct ovpn_peer *peer = NULL; + struct sockaddr_storage ss = { 0 }; + + if (unlikely(!ovpn_peer_skb_to_sockaddr(skb, &ss))) + return NULL; + + if (ovpn->mode == OVPN_MODE_P2P) + peer = ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss); + + return peer; +} + +/** + * ovpn_peer_get_by_id_p2p - get peer by ID in a P2P instance + * @ovpn: the openvpn instance to search + * @peer_id: the ID of the peer to find + * + * Return: the peer if found or NULL otherwise + */ +static struct ovpn_peer *ovpn_peer_get_by_id_p2p(struct ovpn_struct *ovpn, + u32 peer_id) +{ + struct ovpn_peer *tmp, *peer = NULL; + + rcu_read_lock(); + tmp = rcu_dereference(ovpn->peer); + if (likely(tmp && tmp->id == peer_id && ovpn_peer_hold(tmp))) + peer = tmp; + rcu_read_unlock(); + + return peer; +} + +/** + * ovpn_peer_get_by_id - retrieve peer by ID + * @ovpn: the openvpn instance to search + * @peer_id: the unique peer identifier to match + * + * Return: a pointer to the peer if found or NULL otherwise + */ +struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id) +{ + struct ovpn_peer *peer = NULL; + + if (ovpn->mode == OVPN_MODE_P2P) + peer = ovpn_peer_get_by_id_p2p(ovpn, peer_id); + + return peer; +} + +/** + * ovpn_peer_add_p2p - add peer to related tables in a P2P instance + * @ovpn: the instance to add the peer to + * @peer: the peer to add + * + * Return: 0 on success or a negative error code otherwise + */ +static int ovpn_peer_add_p2p(struct ovpn_struct *ovpn, struct ovpn_peer *peer) +{ + struct ovpn_peer *tmp; + + spin_lock_bh(&ovpn->lock); + /* in p2p mode it is possible to have a single peer only, therefore the + * old one is released and substituted by the new one + */ + tmp = rcu_dereference_protected(ovpn->peer, + lockdep_is_held(&ovpn->lock)); + if (tmp) { + tmp->delete_reason = OVPN_DEL_PEER_REASON_TEARDOWN; + ovpn_peer_put(tmp); + } + + rcu_assign_pointer(ovpn->peer, peer); + spin_unlock_bh(&ovpn->lock); + + return 0; +} + +/** + * ovpn_peer_add - add peer to the related tables + * @ovpn: the openvpn instance the peer belongs to + * @peer: the peer object to add + * + * Assume refcounter was increased by caller + * + * Return: 0 on success or a negative error code otherwise + */ +int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) +{ + switch (ovpn->mode) { + case OVPN_MODE_P2P: + return ovpn_peer_add_p2p(ovpn, peer); + default: + return -EOPNOTSUPP; + } +} + +/** + * ovpn_peer_del_p2p - delete peer from related tables in a P2P instance + * @peer: the peer to delete + * @reason: reason why the peer was deleted (sent to userspace) + * + * Return: 0 on success or a negative error code otherwise + */ +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); + + return -ENOENT; + } + + tmp->delete_reason = reason; + RCU_INIT_POINTER(peer->ovpn->peer, NULL); + ovpn_peer_put(tmp); + + return 0; +} + +/** + * ovpn_peer_release_p2p - release peer upon P2P device teardown + * @ovpn: the instance being torn down + */ +void ovpn_peer_release_p2p(struct ovpn_struct *ovpn) +{ + struct ovpn_peer *tmp; + + spin_lock_bh(&ovpn->lock); + tmp = rcu_dereference_protected(ovpn->peer, + lockdep_is_held(&ovpn->lock)); + if (tmp) + ovpn_peer_del_p2p(tmp, OVPN_DEL_PEER_REASON_TEARDOWN); + spin_unlock_bh(&ovpn->lock); +} + +/** + * ovpn_peer_del - delete peer from related tables + * @peer: the peer object to delete + * @reason: reason for deleting peer (will be sent to userspace) + * + * Return: 0 on success or a negative error code otherwise + */ +int ovpn_peer_del(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason) +{ + switch (peer->ovpn->mode) { + case OVPN_MODE_P2P: + return ovpn_peer_del_p2p(peer, reason); + default: + return -EOPNOTSUPP; + } +} diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h new file mode 100644 index 0000000000000000000000000000000000000000..6e0c6b14559de886d0677117f5a7ae029214e1f8 --- /dev/null +++ b/drivers/net/ovpn/peer.h @@ -0,0 +1,79 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload + * + * Copyright (C) 2020-2024 OpenVPN, Inc. + * + * Author: James Yonan <james@openvpn.net> + * Antonio Quartulli <antonio@openvpn.net> + */ + +#ifndef _NET_OVPN_OVPNPEER_H_ +#define _NET_OVPN_OVPNPEER_H_ + +#include <net/dst_cache.h> + +/** + * struct ovpn_peer - the main remote peer object + * @ovpn: main openvpn instance this peer belongs to + * @id: unique identifier + * @vpn_addrs: IP addresses assigned over the tunnel + * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel + * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel + * @dst_cache: cache for dst_entry used to send to peer + * @bind: remote peer binding + * @halt: true if ovpn_peer_mark_delete was called + * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..) + * @lock: protects binding to peer (bind) + * @refcount: reference counter + * @rcu: used to free peer in an RCU safe way + * @delete_work: deferred cleanup work, used to notify userspace + */ +struct ovpn_peer { + struct ovpn_struct *ovpn; + u32 id; + struct { + struct in_addr ipv4; + struct in6_addr ipv6; + } vpn_addrs; + struct dst_cache dst_cache; + struct ovpn_bind __rcu *bind; + bool halt; + enum ovpn_del_peer_reason delete_reason; + spinlock_t lock; /* protects bind */ + struct kref refcount; + struct rcu_head rcu; + struct work_struct delete_work; +}; + +/** + * ovpn_peer_hold - increase reference counter + * @peer: the peer whose counter should be increased + * + * Return: true if the counter was increased or false if it was zero already + */ +static inline bool ovpn_peer_hold(struct ovpn_peer *peer) +{ + return kref_get_unless_zero(&peer->refcount); +} + +void ovpn_peer_release_kref(struct kref *kref); + +/** + * ovpn_peer_put - decrease reference counter + * @peer: the peer whose counter should be decreased + */ +static inline void ovpn_peer_put(struct ovpn_peer *peer) +{ + kref_put(&peer->refcount, ovpn_peer_release_kref); +} + +struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id); +int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer); +int ovpn_peer_del(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason); +void ovpn_peer_release_p2p(struct ovpn_struct *ovpn); + +struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct *ovpn, + struct sk_buff *skb); +struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id); + +#endif /* _NET_OVPN_OVPNPEER_H_ */
An ovpn_peer object holds the whole status of a remote peer (regardless whether it is a server or a client). This includes status for crypto, tx/rx buffers, napi, etc. Only support for one peer is introduced (P2P mode). Multi peer support is introduced with a later patch. Along with the ovpn_peer, also the ovpn_bind object is introcued as the two are strictly related. An ovpn_bind object wraps a sockaddr representing the local coordinates being used to talk to a specific peer. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/Makefile | 2 + drivers/net/ovpn/bind.c | 58 +++++++ drivers/net/ovpn/bind.h | 117 ++++++++++++++ drivers/net/ovpn/main.c | 11 ++ drivers/net/ovpn/main.h | 2 + drivers/net/ovpn/ovpnstruct.h | 4 + drivers/net/ovpn/peer.c | 354 ++++++++++++++++++++++++++++++++++++++++++ drivers/net/ovpn/peer.h | 79 ++++++++++ 8 files changed, 627 insertions(+)