ovpn: fix UDP route cache and endpoint handling
Subsystems (3)
Ralf Lici
reviewercover ovpn: fix UDP route cache and endpoint handling
Hi,
This series fixes several related issues in ovpn's UDP endpoint and route-cache handling.
It preserves IPv6 scope IDs supplied through netlink, handles unspecified source addresses correctly, invalidates cached routes when mutable socket route inputs change, avoids in-place updates of RCU-published peer binds, and prevents stale IPv6 routes from being cached across FIB updates.
Support for honoring UDP sockets bound to a device or local address will follow separately for net-next, after these route-cache fixes are available there.
Cheers,
Ralf Lici
Mandelbit Srl
---
Ralf Lici (5):
ovpn: preserve IPv6 scope id for netlink peer endpoints
ovpn: skip UDP source validation for unspecified addresses
ovpn: track UDP socket route key for peer dst cache
ovpn: avoid in-place updates of peer bind local address
ovpn: avoid caching stale IPv6 dst after FIB changes
drivers/net/ovpn/netlink.c | 6 ++ drivers/net/ovpn/peer.c | 37 ++++--- drivers/net/ovpn/peer.h | 19 +++- drivers/net/ovpn/udp.c | 212 ++++++++++++++++++++++++++++++------- include/net/dst_cache.h | 13 +++ net/core/dst_cache.c | 16 ++- 6 files changed, 248 insertions(+), 55 deletions(-)
Signature · 8 lines
2.54.0 _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Ralf Lici
reviewerPATCH 1/5 ovpn: preserve IPv6 scope id for netlink peer endpoints
ovpn accepts OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID and reports bind->remote.in6.sin6_scope_id in peer dumps, but the netlink endpoint parser never copied the attribute into the sockaddr_in6 used to create or update the peer bind.
As a result, an IPv6 link-local remote endpoint configured through netlink loses its interface scope, unlike on the peer float path where ipv6_iface_scope_id populates the field. The UDPv6 output path then builds a flow with flowi6_oif set to zero and route lookup can fail or select the wrong interface.
Copy the scope id when parsing non-v4-mapped IPv6 remote endpoints. The existing precheck already rejects the scope-id attribute for IPv4 and v4-mapped IPv6 remotes.
--- drivers/net/ovpn/netlink.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/net/ovpn/netlink.c +6 -0
@@ -100,6 +100,8 @@ static bool ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs,struct sockaddr_in6 *sin6;struct sockaddr_in *sin;struct in6_addr *in6; +struct nlattr *scope; +u32 scope_id =0 ;__be16 port =0 ;__be32 *in;
@@ -114,6 +116,9 @@ static bool ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs, }else if (attrs[OVPN_A_PEER_REMOTE_IPV6 ]) { ss->ss_family =AF_INET6 ; in6 =nla_data (attrs[OVPN_A_PEER_REMOTE_IPV6 ]); + scope = attrs[OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID ]; +if (scope) + scope_id =nla_get_u32 (scope); }else {return false; }
@@ -126,6 +131,7 @@ static bool ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs,if (!ipv6_addr_v4mapped (in6)) { sin6 = (struct sockaddr_in6 *)ss; sin6->sin6_port = port; + sin6->sin6_scope_id = scope_id;memcpy (&sin6->sin6_addr, in6,sizeof (*in6));break ; }
Signature · 8 lines
2.54.0 _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Ralf Lici
reviewerPATCH 2/5 ovpn: skip UDP source validation for unspecified addresses
In reply to Ralf Liciovpn validates the cached local UDP source address before reusing or refreshing a peer dst cache. This is only meaningful when a concrete source address is selected.
For IPv6, calling ipv6_chk_addr with :: checks whether the unspecified address itself is configured on the host. A peer may legitimately have bind->local.ipv6 set to :: when no local endpoint was configured or after a stale learned address was cleared. In that case the source should be left unspecified and selected by ip6_dst_lookup_flow().
For IPv4, inet_confirm_addr(..., local = 0, ...) asks for local address autoselection rather than validating a chosen source. Skip the precheck there as well and let ip_route_output_flow select or reject the source.
Only validate non-zero/non-any source addresses.
--- drivers/net/ovpn/udp.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/net/ovpn/udp.c +4 -3
@@ -161,8 +161,8 @@ static int ovpn_udp4_output(struct ovpn_peer *peer, struct ovpn_bind *bind,if (rt) goto transmit; -if (unlikely (!inet_confirm_addr (sock_net (sk),NULL ,0 , fl.saddr, -RT_SCOPE_HOST ))) { +if (fl.saddr &&unlikely (!inet_confirm_addr (sock_net (sk),NULL ,0 , + fl.saddr,RT_SCOPE_HOST ))) {/* we may end up here when the cached address is not usable * anymore. In this case we reset address/cache and perform a * new look up
@@ -238,7 +238,8 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,if (dst) goto transmit; -if (unlikely (!ipv6_chk_addr (sock_net (sk), &fl.saddr,NULL ,0 ))) { +if (!ipv6_addr_any (&fl.saddr) && +unlikely (!ipv6_chk_addr (sock_net (sk), &fl.saddr,NULL ,0 ))) {/* we may end up here when the cached address is not usable * anymore. In this case we reset address/cache and perform a * new look up
Signature · 8 lines
2.54.0 _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Ralf Lici
reviewerPATCH 3/5 ovpn: track UDP socket route key for peer dst cache
In reply to Ralf Liciovpn stores the route used to transmit UDP packets in a per-peer dst cache. A cached dst is only valid for the route lookup inputs used when it was resolved.
Some of those inputs are mutable while userspace still owns the UDP socket. In particular, changes to the socket mark or UDP source port do not invalidate ovpn's peer dst cache, so ovpn can keep using a route selected with an old socket route key.
Replace the cached mark with a route key containing the socket-owned lookup inputs currently used by ovpn, and reset the peer dst cache when the key changes. Before storing a newly looked-up dst, recheck the route key under the peer lock so a dst resolved for stale socket state is not published.
--- drivers/net/ovpn/peer.c | 1 + drivers/net/ovpn/peer.h | 19 ++++++++- drivers/net/ovpn/udp.c | 90 +++++++++++++++++++++++++++++++++++------ 3 files changed, 95 insertions(+), 15 deletions(-)
drivers/net/ovpn/peer.c +1 -0
@@ -112,6 +112,7 @@ struct ovpn_peer *ovpn_peer_new(struct ovpn_priv *ovpn, u32 id)RCU_INIT_POINTER (peer->bind,NULL );ovpn_crypto_state_init (&peer->crypto);spin_lock_init (&peer->lock); +seqcount_spinlock_init (&peer->route_key_seq, &peer->lock);kref_init (&peer->refcount);ovpn_peer_stats_init (&peer->vpn_stats);ovpn_peer_stats_init (&peer->link_stats);
drivers/net/ovpn/peer.h +17 -2
@@ -10,6 +10,7 @@#ifndef _NET_OVPN_OVPNPEER_H_#define _NET_OVPN_OVPNPEER_H_ +#include <linux/seqlock.h> #include <net/dst_cache.h> #include <net/strparser.h>
@@ -17,6 +18,16 @@#include "socket.h" #include "stats.h" +/** +* struct ovpn_route_key - route key used for the peer dst cache +* @mark: fwmark used for route lookup +* @sport: UDP source port used for route lookup +*/ +struct ovpn_route_key { +u32 mark; +__be16 sport; +}; +/** * struct ovpn_peer - the main remote peer object * @ovpn: main openvpn instance this peer belongs to
@@ -45,6 +56,8 @@* @tcp.sk_cb.ops: pointer to the original prot_ops object (TCP only) * @crypto: the crypto configuration (ciphers, keys, etc..) * @dst_cache: cache for dst_entry used to send to peer +* @route_key: route key matching the current dst cache contents +* @route_key_seq: seqcount protecting lockless route_key reads * @bind: remote peer binding * @keepalive_interval: seconds after which a new keepalive should be sent * @keepalive_xmit_exp: future timestamp when next keepalive should be sent
@@ -55,7 +68,7 @@* @vpn_stats: per-peer in-VPN TX/RX stats * @link_stats: per-peer link/transport TX/RX stats * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..) -* @lock: protects binding to peer (bind) and keepalive* fields +* @lock: protects binding to peer (bind), route_key and keepalive* fields * @refcount: reference counter * @rcu: used to free peer in an RCU safe way * @release_entry: entry for the socket release list
@@ -99,6 +112,8 @@ struct ovpn_peer { } tcp;struct ovpn_crypto_state crypto;struct dst_cache dst_cache; +struct ovpn_route_key route_key; +seqcount_spinlock_t route_key_seq;struct ovpn_bind __rcu *bind;unsigned long keepalive_interval;unsigned long keepalive_xmit_exp;
@@ -109,7 +124,7 @@ struct ovpn_peer {struct ovpn_peer_stats vpn_stats;struct ovpn_peer_stats link_stats;enum ovpn_del_peer_reason delete_reason; -spinlock_t lock;/* protects bind and keepalive* */ +spinlock_t lock;/* protects bind, route_key and keepalive* */ struct kref refcount;struct rcu_head rcu;struct llist_node release_entry;
drivers/net/ovpn/udp.c +77 -13
179 lines not on this page · raw message
Signature · 8 lines
2.54.0 _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Ralf Lici
reviewerPATCH 4/5 ovpn: avoid in-place updates of peer bind local address
In reply to Ralf Licistruct ovpn_bind is published through peer->bind with RCU. Remote endpoint changes already replace the whole bind object, but local endpoint learning and UDP source fallback still updated bind->local in place. UDP TX can read it locklessly while another CPU updates it under peer->lock. For IPv6, that can produce torn reads of the address field.
Fix this by making the local endpoint immutable after publication too:
build a new bind object with the updated local address and publish it through peer->bind.
When UDP TX discovers that the remembered local source is no longer usable, retry route lookup with a wildcard source. If the lookup succeeds and the bind used for the lookup is still current, invalidate the peer dst cache and best-effort publish a replacement bind with wildcard local address. The current packet can still be transmitted with the resolved route even if that bind replacement fails; a later cache miss will retry the repair.
Only store the resolved dst when the local address did not need to be reset. A local address change invalidates all per-CPU dst cache entries, while dst_cache_set_ip4 and dst_cache_set_ip6 only update the current CPU slot. Avoid the old reset-then-set pattern and let the next TX repopulate the cache from the new bind state.
--- drivers/net/ovpn/peer.c | 36 +++++++++----- drivers/net/ovpn/udp.c | 108 +++++++++++++++++++++++++++++++--------- 2 files changed, 107 insertions(+), 37 deletions(-)
drivers/net/ovpn/peer.c +23 -13
@@ -197,12 +197,11 @@ int ovpn_peer_reset_sockaddr(struct ovpn_peer *peer,void ovpn_peer_endpoints_update (struct ovpn_peer *peer,struct sk_buff *skb) {struct hlist_nulls_head *nhead; +const void *local_ip =NULL ;struct sockaddr_storage ss;struct sockaddr_in6 *sa6; -bool reset_cache = false;struct sockaddr_in *sa;struct ovpn_bind *bind; -const void *local_ip;size_t salen =0 ;spin_lock_bh (&peer->lock);
@@ -224,7 +223,6 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb) sa->sin_addr.s_addr =ip_hdr (skb)->saddr; sa->sin_port =udp_hdr (skb)->source; salen =sizeof (*sa); - reset_cache = true;break ; }
@@ -236,8 +234,7 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)netdev_name (peer->ovpn->dev), peer->id, &bind->local.ipv4.s_addr, &ip_hdr(skb)->daddr); - bind->local.ipv4.s_addr =ip_hdr (skb)->daddr; - reset_cache = true; + local_ip = &ip_hdr (skb)->daddr; }break ;case htons (ETH_P_IPV6 ):
@@ -254,7 +251,6 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb) sa6->sin6_scope_id =ipv6_iface_scope_id (&ipv6_hdr (skb)->saddr, skb->skb_iif); salen =sizeof (*sa6); - reset_cache = true;break ; }
@@ -267,26 +263,40 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)netdev_name (peer->ovpn->dev), peer->id, &bind->local.ipv6, &ipv6_hdr(skb)->daddr); - bind->local.ipv6 =ipv6_hdr (skb)->daddr; - reset_cache = true; + local_ip = &ipv6_hdr (skb)->daddr; }break ;default : goto unlock; } -if (unlikely (reset_cache)) -dst_cache_reset (&peer->dst_cache); - -/* if the peer did not float, we can bail out now */ -if (likely (!salen)) +/* if there was no float and the local address is unchanged, bail out */ +if (likely (!salen && !local_ip)) goto unlock; +/* if only the local address changed, populate ss with the current +* remote +*/ +if (!salen) +memcpy (&ss, &bind->remote, + bind->remote.in4.sin_family ==AF_INET ? +sizeof (struct sockaddr_in ) : +sizeof (struct sockaddr_in6 )); +if (unlikely (ovpn_peer_reset_sockaddr (peer, (struct sockaddr_storage *)&ss, local_ip) <0 )) goto unlock; +/* reset the cache only after a successful bind update to avoid useless +* cache misses on concurrent TX +*/ +dst_cache_reset (&peer->dst_cache); + +/* if the peer did not float, we can bail out now */ +if (!salen) + goto unlock; +net_dbg_ratelimited ("%s: peer %d floated to %pIScp" ,netdev_name (peer->ovpn->dev), peer->id, &ss);
drivers/net/ovpn/udp.c +84 -24
161 lines not on this page · raw message
Signature · 8 lines
2.54.0 _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Ralf Lici
reviewerPATCH 5/5 ovpn: avoid caching stale IPv6 dst after FIB changes
In reply to Ralf Liciovpn stores the IPv6 route used for UDP transmission in a per-peer dst cache. IPv6 dst validation uses a cookie derived from the route itself, or, for routes without their own sernum, from the associated fib6 node.
If the IPv6 FIB changes after ip6_dst_lookup_flow returns but before dst_cache_set_ip6 reads the cookie, ovpn can store an old dst with a new cookie. Later dst_cache_get_ip6 can then consider that stale dst valid because the stored cookie matches the updated fib6 node sernum.
Sample the IPv6 FIB generation before and after route lookup, and only populate ovpn's peer dst cache if the generation did not change while the lookup was in flight. Also add a dst_cache helper that stores a caller-provided IPv6 cookie, so the cached dst carries the cookie sampled from the lookup result instead of one read after a concurrent FIB update.
The current packet may still be transmitted with the route returned by the lookup if the FIB changes before TX completion. This patch only prevents that potentially stale route from being preserved in ovpn's peer dst cache and reused for later packets.
--- drivers/net/ovpn/udp.c | 45 +++++++++++++++++++++++++++-------------- include/net/dst_cache.h | 13 ++++++++++++ net/core/dst_cache.c | 16 +++++++++++---- 3 files changed, 55 insertions(+), 19 deletions(-)
drivers/net/ovpn/udp.c +30 -15
@@ -316,9 +316,11 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind, {struct in6_addr local = in6addr_any;struct sockaddr_storage remote; +struct net *net =sock_net (sk);bool reset_local = false;struct dst_entry *dst; -int ret; +int gen0, gen1, ret; +u32 cookie;struct flowi6 fl = { .saddr = bind->local.ipv6,
@@ -344,7 +346,9 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind, reset_local = true; } - dst =ip6_dst_lookup_flow (sock_net (sk), sk, &fl,NULL ); + gen0 =rt_genid_ipv6 (net); + + dst =ip6_dst_lookup_flow (net, sk, &fl,NULL );if (IS_ERR (dst)) { ret =PTR_ERR (dst); net_dbg_ratelimited("%s: no route to host %pISpc: %d\n",
@@ -353,27 +357,38 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind, goto err; } + cookie =rt6_get_cookie (dst_rt6_info (dst)); + gen1 =rt_genid_ipv6 (net); +/* avoid storing a stale cache or local address */ spin_lock_bh (&peer->lock);if (likely (ovpn_dst_cache_current (peer, bind, key))) { -if (!reset_local) { -dst_cache_set_ip6 (cache, dst, &fl.saddr); +/* cache the dst with the original cookie only if the learned +* local source was not reset and the FIB did not change +*/ +if (!reset_local &&likely (gen0 == gen1)) { +dst_cache_set_ip6_cookie (cache, dst, &fl.saddr, cookie);spin_unlock_bh (&peer->lock); goto transmit; } -/* invalidate per-CPU dst entries that may still carry -* the stale source -*/ -dst_cache_reset (cache); +if (reset_local) { +/* invalidate per-CPU dst entries that may still carry +* the stale source +*/ +dst_cache_reset (cache); + +/* preserve the current remote */ +memcpy (&remote, &bind->remote, +sizeof (struct sockaddr_in6 )); +/* The current packet already has a valid +* wildcard-source route. If replacing the bind fails, +* leave the stale local in place; a later cache miss +* will retry the repair. +*/ +ovpn_peer_reset_sockaddr (peer, &remote, &local); + } -/* preserve the current remote */ -memcpy (&remote, &bind->remote,sizeof (struct sockaddr_in6 )); -/* The current packet already has a valid wildcard-source route. -* If replacing the bind fails, leave the stale local in place; -* a later cache miss will retry the repair. -*/ -ovpn_peer_reset_sockaddr (peer, &remote, &local); }spin_unlock_bh (&peer->lock);
include/net/dst_cache.h +13 -0
@@ -45,6 +45,19 @@ void dst_cache_set_ip4(struct dst_cache *dst_cache, struct dst_entry *dst,#if IS_ENABLED (CONFIG_IPV6 ) +/** +* dst_cache_set_ip6_cookie - store ipv6 dst with caller-provided cookie +* @dst_cache: the cache +* @dst: the entry to be cached +* @saddr: the source address to be stored inside the cache +* @cookie: the route validation cookie to store with @dst +* +* local BH must be disabled. +*/ +void dst_cache_set_ip6_cookie (struct dst_cache *dst_cache, +struct dst_entry *dst, +const struct in6_addr *saddr,u32 cookie); +/** * dst_cache_set_ip6 - store the ipv6 dst into the cache * @dst_cache: the cache
net/core/dst_cache.c +12 -4
@@ -117,8 +117,9 @@ void dst_cache_set_ip4(struct dst_cache *dst_cache, struct dst_entry *dst,EXPORT_SYMBOL_GPL (dst_cache_set_ip4);#if IS_ENABLED (CONFIG_IPV6 ) -void dst_cache_set_ip6 (struct dst_cache *dst_cache,struct dst_entry *dst, -const struct in6_addr *saddr) +void dst_cache_set_ip6_cookie (struct dst_cache *dst_cache, +struct dst_entry *dst, +const struct in6_addr *saddr,u32 cookie) {struct dst_cache_pcpu *idst;
@@ -128,11 +129,18 @@ void dst_cache_set_ip6(struct dst_cache *dst_cache, struct dst_entry *dst,local_lock_nested_bh (&dst_cache->cache->bh_lock); idst =this_cpu_ptr (dst_cache->cache); -dst_cache_per_cpu_dst_set (idst, dst, -rt6_get_cookie (dst_rt6_info (dst))); +dst_cache_per_cpu_dst_set (idst, dst, cookie); idst->in6_saddr = *saddr;local_unlock_nested_bh (&dst_cache->cache->bh_lock); } +EXPORT_SYMBOL_GPL (dst_cache_set_ip6_cookie); + +void dst_cache_set_ip6 (struct dst_cache *dst_cache,struct dst_entry *dst, +const struct in6_addr *saddr) +{ +dst_cache_set_ip6_cookie (dst_cache, dst, saddr, +rt6_get_cookie (dst_rt6_info (dst))); +}EXPORT_SYMBOL_GPL (dst_cache_set_ip6); struct dst_entry *dst_cache_get_ip6(struct dst_cache *dst_cache,
Signature · 8 lines
2.54.0 _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel