In a multi-peer scenario there are a number of situations when a
specific peer needs to be looked up.
We may want to lookup a peer by:
1. its ID
2. its VPN destination IP
3. its transport IP/port couple
For each of the above, there is a specific routing table referencing all
peers for fast look up.
Case 2. is a bit special in the sense that an outgoing packet may not be
sent to the peer VPN IP directly, but rather to a network behind it. For
this reason we first perform a nexthop lookup in the system routing
table and then we use the retrieved nexthop as peer search key.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
drivers/net/ovpn/peer.c | 297 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 288 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index 155b8571467274e068bbe30ce199667c4b58717d..976338c53913d513fd316d5ec51e6f35c21ecbfc 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -10,6 +10,7 @@
#include <linux/skbuff.h>
#include <linux/list.h>
#include <linux/hashtable.h>
+#include <net/ip6_route.h>
#include "ovpnstruct.h"
#include "bind.h"
@@ -136,6 +137,118 @@ static int ovpn_peer_skb_to_sockaddr(struct sk_buff *skb,
return -1;
}
+/**
+ * ovpn_nexthop_from_skb4 - retrieve IPv4 nexthop for outgoing skb
+ * @skb: the outgoing packet
+ *
+ * Return: the IPv4 of the nexthop
+ */
+static __be32 ovpn_nexthop_from_skb4(struct sk_buff *skb)
+{
+ const struct rtable *rt = skb_rtable(skb);
+
+ if (rt && rt->rt_uses_gateway)
+ return rt->rt_gw4;
+
+ return ip_hdr(skb)->daddr;
+}
+
+/**
+ * ovpn_nexthop_from_skb6 - retrieve IPv6 nexthop for outgoing skb
+ * @skb: the outgoing packet
+ *
+ * Return: the IPv6 of the nexthop
+ */
+static struct in6_addr ovpn_nexthop_from_skb6(struct sk_buff *skb)
+{
+ const struct rt6_info *rt = skb_rt6_info(skb);
+
+ if (!rt || !(rt->rt6i_flags & RTF_GATEWAY))
+ return ipv6_hdr(skb)->daddr;
+
+ return rt->rt6i_gateway;
+}
+
+#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)]; \
+})
+
+/**
+ * ovpn_peer_get_by_vpn_addr4 - retrieve peer by its VPN IPv4 address
+ * @ovpn: the openvpn instance to search
+ * @addr: VPN IPv4 to use as search key
+ *
+ * Refcounter is not increased for the returned peer.
+ *
+ * Return: the peer if found or NULL otherwise
+ */
+static struct ovpn_peer *ovpn_peer_get_by_vpn_addr4(struct ovpn_priv *ovpn,
+ __be32 addr)
+{
+ struct hlist_nulls_head *nhead;
+ struct hlist_nulls_node *ntmp;
+ struct ovpn_peer *tmp;
+ unsigned int slot;
+
+begin:
+ slot = ovpn_get_hash_slot(&addr, sizeof(addr),
+ ovpn->peers->by_vpn_addr);
+ nhead = &ovpn->peers->by_vpn_addr[slot];
+
+ hlist_nulls_for_each_entry_rcu(tmp, ntmp, nhead, hash_entry_addr4)
+ if (addr == tmp->vpn_addrs.ipv4.s_addr)
+ return tmp;
+
+ /* item may have moved during lookup - check nulls and restart
+ * if that's the case
+ */
+ if (get_nulls_value(ntmp) != slot)
+ goto begin;
+
+ return NULL;
+}
+
+/**
+ * ovpn_peer_get_by_vpn_addr6 - retrieve peer by its VPN IPv6 address
+ * @ovpn: the openvpn instance to search
+ * @addr: VPN IPv6 to use as search key
+ *
+ * Refcounter is not increased for the returned peer.
+ *
+ * Return: the peer if found or NULL otherwise
+ */
+static struct ovpn_peer *ovpn_peer_get_by_vpn_addr6(struct ovpn_priv *ovpn,
+ struct in6_addr *addr)
+{
+ struct hlist_nulls_head *nhead;
+ struct hlist_nulls_node *ntmp;
+ struct ovpn_peer *tmp;
+ unsigned int slot;
+
+begin:
+ slot = ovpn_get_hash_slot(addr, sizeof(*addr),
+ ovpn->peers->by_vpn_addr);
+ nhead = &ovpn->peers->by_vpn_addr[slot];
+
+ hlist_nulls_for_each_entry_rcu(tmp, ntmp, nhead, hash_entry_addr6)
+ if (ipv6_addr_equal(addr, &tmp->vpn_addrs.ipv6))
+ return tmp;
+
+ /* item may have moved during lookup - check nulls and restart
+ * if that's the case
+ */
+ if (get_nulls_value(ntmp) != slot)
+ goto begin;
+
+ return NULL;
+}
+
/**
* ovpn_peer_transp_match - check if sockaddr and peer binding match
* @peer: the peer to get the binding from
@@ -213,14 +326,43 @@ ovpn_peer_get_by_transp_addr_p2p(struct ovpn_priv *ovpn,
struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_priv *ovpn,
struct sk_buff *skb)
{
- struct ovpn_peer *peer = NULL;
+ struct ovpn_peer *tmp, *peer = NULL;
struct sockaddr_storage ss = { 0 };
+ struct hlist_nulls_head *nhead;
+ struct hlist_nulls_node *ntmp;
+ unsigned int slot;
+ size_t sa_len;
- if (unlikely(!ovpn_peer_skb_to_sockaddr(skb, &ss)))
+ sa_len = ovpn_peer_skb_to_sockaddr(skb, &ss);
+ if (unlikely(sa_len < 0))
return NULL;
if (ovpn->mode == OVPN_MODE_P2P)
- peer = ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss);
+ return ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss);
+
+ rcu_read_lock();
+begin:
+ slot = ovpn_get_hash_slot(&ss, sa_len, ovpn->peers->by_transp_addr);
+ nhead = &ovpn->peers->by_transp_addr[slot];
+
+ hlist_nulls_for_each_entry_rcu(tmp, ntmp, nhead,
+ hash_entry_transp_addr) {
+ if (!ovpn_peer_transp_match(tmp, &ss))
+ continue;
+
+ if (!ovpn_peer_hold(tmp))
+ continue;
+
+ peer = tmp;
+ break;
+ }
+
+ /* item may have moved during lookup - check nulls and restart
+ * if that's the case
+ */
+ if (!peer && get_nulls_value(ntmp) != slot)
+ goto begin;
+ rcu_read_unlock();
return peer;
}
@@ -255,10 +397,27 @@ static struct ovpn_peer *ovpn_peer_get_by_id_p2p(struct ovpn_priv *ovpn,
*/
struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_priv *ovpn, u32 peer_id)
{
- struct ovpn_peer *peer = NULL;
+ struct ovpn_peer *tmp, *peer = NULL;
+ struct hlist_head *head;
if (ovpn->mode == OVPN_MODE_P2P)
- peer = ovpn_peer_get_by_id_p2p(ovpn, peer_id);
+ return ovpn_peer_get_by_id_p2p(ovpn, peer_id);
+
+ head = ovpn_get_hash_head(ovpn->peers->by_id, &peer_id,
+ sizeof(peer_id));
+
+ rcu_read_lock();
+ hlist_for_each_entry_rcu(tmp, head, hash_entry_id) {
+ if (tmp->id != peer_id)
+ continue;
+
+ if (!ovpn_peer_hold(tmp))
+ continue;
+
+ peer = tmp;
+ break;
+ }
+ rcu_read_unlock();
return peer;
}
@@ -280,6 +439,8 @@ struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_priv *ovpn,
struct sk_buff *skb)
{
struct ovpn_peer *peer = NULL;
+ struct in6_addr addr6;
+ __be32 addr4;
/* in P2P mode, no matter the destination, packets are always sent to
* the single peer listening on the other side
@@ -290,11 +451,109 @@ struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_priv *ovpn,
if (unlikely(peer && !ovpn_peer_hold(peer)))
peer = NULL;
rcu_read_unlock();
+ return peer;
}
+ rcu_read_lock();
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ addr4 = ovpn_nexthop_from_skb4(skb);
+ peer = ovpn_peer_get_by_vpn_addr4(ovpn, addr4);
+ break;
+ case htons(ETH_P_IPV6):
+ addr6 = ovpn_nexthop_from_skb6(skb);
+ peer = ovpn_peer_get_by_vpn_addr6(ovpn, &addr6);
+ break;
+ }
+
+ if (unlikely(peer && !ovpn_peer_hold(peer)))
+ peer = NULL;
+ rcu_read_unlock();
+
return peer;
}
+/**
+ * ovpn_nexthop_from_rt4 - look up the IPv4 nexthop for the given destination
+ * @ovpn: the private data representing the current VPN session
+ * @dest: the destination to be looked up
+ *
+ * Looks up in the IPv4 system routing table the IP of the nexthop to be used
+ * to reach the destination passed as argument. If no nexthop can be found, the
+ * destination itself is returned as it probably has to be used as nexthop.
+ *
+ * Return: the IP of the next hop if found or dest itself otherwise
+ */
+static __be32 ovpn_nexthop_from_rt4(struct ovpn_priv *ovpn, __be32 dest)
+{
+ struct rtable *rt;
+ struct flowi4 fl = {
+ .daddr = dest
+ };
+
+ rt = ip_route_output_flow(dev_net(ovpn->dev), &fl, NULL);
+ if (IS_ERR(rt)) {
+ net_dbg_ratelimited("%s: no route to host %pI4\n",
+ netdev_name(ovpn->dev), &dest);
+ /* if we end up here this packet is probably going to be
+ * thrown away later
+ */
+ return dest;
+ }
+
+ if (!rt->rt_uses_gateway)
+ goto out;
+
+ dest = rt->rt_gw4;
+out:
+ ip_rt_put(rt);
+ return dest;
+}
+
+/**
+ * ovpn_nexthop_from_rt6 - look up the IPv6 nexthop for the given destination
+ * @ovpn: the private data representing the current VPN session
+ * @dest: the destination to be looked up
+ *
+ * Looks up in the IPv6 system routing table the IP of the nexthop to be used
+ * to reach the destination passed as argument. If no nexthop can be found, the
+ * destination itself is returned as it probably has to be used as nexthop.
+ *
+ * Return: the IP of the next hop if found or dest itself otherwise
+ */
+static struct in6_addr ovpn_nexthop_from_rt6(struct ovpn_priv *ovpn,
+ struct in6_addr dest)
+{
+#if IS_ENABLED(CONFIG_IPV6)
+ struct dst_entry *entry;
+ struct rt6_info *rt;
+ struct flowi6 fl = {
+ .daddr = dest,
+ };
+
+ entry = ipv6_stub->ipv6_dst_lookup_flow(dev_net(ovpn->dev), NULL, &fl,
+ NULL);
+ if (IS_ERR(entry)) {
+ net_dbg_ratelimited("%s: no route to host %pI6c\n",
+ netdev_name(ovpn->dev), &dest);
+ /* if we end up here this packet is probably going to be
+ * thrown away later
+ */
+ return dest;
+ }
+
+ rt = dst_rt6_info(entry);
+
+ if (!(rt->rt6i_flags & RTF_GATEWAY))
+ goto out;
+
+ dest = rt->rt6i_gateway;
+out:
+ dst_release((struct dst_entry *)rt);
+#endif
+ return dest;
+}
+
/**
* ovpn_peer_check_by_src - check that skb source is routed via peer
* @ovpn: the openvpn instance to search
@@ -307,6 +566,8 @@ bool ovpn_peer_check_by_src(struct ovpn_priv *ovpn, struct sk_buff *skb,
struct ovpn_peer *peer)
{
bool match = false;
+ struct in6_addr addr6;
+ __be32 addr4;
if (ovpn->mode == OVPN_MODE_P2P) {
/* in P2P mode, no matter the destination, packets are always
@@ -315,15 +576,33 @@ bool ovpn_peer_check_by_src(struct ovpn_priv *ovpn, struct sk_buff *skb,
rcu_read_lock();
match = (peer == rcu_dereference(ovpn->peer));
rcu_read_unlock();
+ return match;
+ }
+
+ /* This function performs a reverse path check, therefore we now
+ * lookup the nexthop we would use if we wanted to route a packet
+ * to the source IP. If the nexthop matches the sender we know the
+ * latter is valid and we allow the packet to come in
+ */
+
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ addr4 = ovpn_nexthop_from_rt4(ovpn, ip_hdr(skb)->saddr);
+ rcu_read_lock();
+ match = (peer == ovpn_peer_get_by_vpn_addr4(ovpn, addr4));
+ rcu_read_unlock();
+ break;
+ case htons(ETH_P_IPV6):
+ addr6 = ovpn_nexthop_from_rt6(ovpn, ipv6_hdr(skb)->saddr);
+ rcu_read_lock();
+ match = (peer == ovpn_peer_get_by_vpn_addr6(ovpn, &addr6));
+ rcu_read_unlock();
+ break;
}
return match;
}
-#define ovpn_get_hash_head(_tbl, _key, _key_len) ({ \
- typeof(_tbl) *__tbl = &(_tbl); \
- (&(*__tbl)[jhash(_key, _key_len, 0) % HASH_SIZE(*__tbl)]); }) \
-
/**
* ovpn_peer_add_mp - add peer to related tables in a MP instance
* @ovpn: the instance to add the peer to
--
2.45.2On 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.
Regards,
--
Antonio Quartulli
OpenVPN Inc.
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).
--
Sabrina
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.
Regards,
--
Antonio Quartulli
OpenVPN Inc.
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,
>
>
--
Antonio Quartulli
OpenVPN Inc.
© 2016 - 2026 Red Hat, Inc.