From: Antonio Quartulli <[email protected]>
ovpn_udp{4,6}_output() resolve a route from a flow key sampled from the
peer binding and the transport socket, then cache the result in the
per-peer dst_cache. Several of those sources may change concurrently
with TX, and the dst_cache epoch (reset_ts vs the per-CPU refresh_ts
stamped at get-miss time) only neutralizes the common ordering. Three
issues remain:
- ovpn_peer_endpoints_update() may either update bind->local in place
or replace the whole bind via RCU (float -> new remote, hence new
daddr/dport/oif). It already dst_cache_reset()s, but the TX path can
still cache a dst it resolved with the pre-update values if its
dst_cache_get-miss lands a strictly later jiffy than the reset.
- inet_sk(sk)->inet_sport can be reset to 0 by __udp_disconnect()
(connect() with AF_UNSPEC) on a socket without SOCK_BINDPORT_LOCK,
and sk->sk_mark can change any time via setsockopt(SO_MARK). Neither
triggers an ovpn cache reset, so a previously-cached entry resolved
with the old value persists until dst obsolescence. Both fields are
also read locklessly into the flow key (data race).
- A sport of 0 means the transport socket has been disconnected and
unhashed; sending a UDP packet from source port 0 is nonsense.
In the common dispatcher ovpn_udp_output() (so every TX, including
cache hits, runs the check):
- Sample inet_sport with READ_ONCE(). If it is 0, emit a one-time
netdev_warn_once() and return -EIO so ovpn_udp_send_skb() drops the
skb.
- Sample sk_mark with READ_ONCE().
- Compare both against the values stored when the dst_cache was last
(re-)populated (new per-peer fields dst_cache_sport/dst_cache_mark,
zero-initialised by kzalloc_obj() in ovpn_peer_new()). On mismatch
dst_cache_reset() the cache and WRITE_ONCE() the new values, so the
subsequent dst_cache_get() misses and the lookup re-resolves with
the current sport/mark.
- Pass sport/mark down to ovpn_udp{4,6}_output(); they use those in
the flowi initializer and skip the per-function sampling.
The post-lookup re-check in the v4/v6 paths covers both the bind/local
race the original commit addressed (rcu_access_pointer on peer->bind
and READ_ONCE/ovpn_peer_local_ipv6 on bind->local) and sport/mark: the
TX-entry check alone is not enough, because a slow resolver can finish
its route lookup after another CPU has already re-tagged the cache with
a different sport/mark, so it must re-verify both before populating the
cache. sk_protocol is immutable post-creation and is intentionally read
plain.
The in-flight packet is still transmitted with the resolved
parameters; only the cache is guarded. No fast-path lock is added.
Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
Signed-off-by: Antonio Quartulli <[email protected]>
---
drivers/net/ovpn/peer.h | 8 ++++
drivers/net/ovpn/udp.c | 101 ++++++++++++++++++++++++++++++++++------
2 files changed, 94 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index c0994c606554..17d57b12fa5e 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h
@@ -46,6 +46,12 @@
* @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
+ * @dst_cache_sport: inet_sport observed when the dst_cache was last
+ * (re-)populated; compared on every TX to detect changes
+ * (e.g. connect(AF_UNSPEC)) and invalidate the cache
+ * @dst_cache_mark: sk_mark observed when the dst_cache was last
+ * (re-)populated; compared on every TX to detect changes
+ * via setsockopt(SO_MARK) and invalidate the cache
* @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
@@ -102,6 +108,8 @@ struct ovpn_peer {
} tcp;
struct ovpn_crypto_state crypto;
struct dst_cache dst_cache;
+ __be16 dst_cache_sport;
+ u32 dst_cache_mark;
struct ovpn_bind __rcu *bind;
unsigned long keepalive_interval;
unsigned long keepalive_xmit_exp;
diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index 17d65d1595ed..fc5ef77d17b0 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -143,19 +143,24 @@ static int ovpn_udp_encap_recv(struct sock *sk, struct
sk_buff *skb)
*/
static int ovpn_udp4_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
struct dst_cache *cache, struct sock *sk,
- struct sk_buff *skb)
+ struct sk_buff *skb, __be16 sport, u32 mark)
{
+ /* bind->local is updated in place under peer->lock; a single aligned
+ * word is read/written atomically via {READ,WRITE}_ONCE. Snapshot it
+ * so the post-lookup validity check can compare against the value we
+ * actually resolved with: fl.saddr may be overwritten by the FIB when
+ * the local address is unset, and comparing bind->local against that
+ * would spuriously skip caching.
+ */
+ __be32 saddr = READ_ONCE(bind->local.ipv4.s_addr);
struct rtable *rt;
struct flowi4 fl = {
- /* bind->local is updated in place under peer->lock; a single
- * aligned word is read/written atomically via {READ,WRITE}_ONCE
- */
- .saddr = READ_ONCE(bind->local.ipv4.s_addr),
+ .saddr = saddr,
.daddr = bind->remote.in4.sin_addr.s_addr,
- .fl4_sport = inet_sk(sk)->inet_sport,
+ .fl4_sport = sport,
.fl4_dport = bind->remote.in4.sin_port,
.flowi4_proto = sk->sk_protocol,
- .flowi4_mark = sk->sk_mark,
+ .flowi4_mark = mark,
};
int ret;
@@ -171,6 +176,7 @@ static int ovpn_udp4_output(struct ovpn_peer *peer, struct
ovpn_bind *bind,
* new look up
*/
fl.saddr = 0;
+ saddr = 0;
spin_lock_bh(&peer->lock);
WRITE_ONCE(bind->local.ipv4.s_addr, 0);
spin_unlock_bh(&peer->lock);
@@ -180,6 +186,7 @@ static int ovpn_udp4_output(struct ovpn_peer *peer, struct
ovpn_bind *bind,
rt = ip_route_output_flow(sock_net(sk), &fl, sk);
if (IS_ERR(rt) && PTR_ERR(rt) == -EINVAL) {
fl.saddr = 0;
+ saddr = 0;
spin_lock_bh(&peer->lock);
WRITE_ONCE(bind->local.ipv4.s_addr, 0);
spin_unlock_bh(&peer->lock);
@@ -196,7 +203,21 @@ static int ovpn_udp4_output(struct ovpn_peer *peer, struct
ovpn_bind *bind,
ret);
goto err;
}
- dst_cache_set_ip4(cache, &rt->dst, fl.saddr);
+ /* only cache the result if the parameters we resolved with are still
+ * current: a concurrent ovpn_peer_endpoints_update() may have replaced
+ * the bind (float) or updated bind->local in place, and a concurrent
+ * ovpn_udp_output() may have re-tagged the cache with a different
+ * sport/mark after we sampled them. In any of these cases the cache was
+ * already reset and re-caching a route resolved with the stale values
+ * would poison it, so bail out and reset instead.
+ */
+ if (rcu_access_pointer(peer->bind) == bind &&
+ READ_ONCE(bind->local.ipv4.s_addr) == saddr &&
+ READ_ONCE(peer->dst_cache_sport) == sport &&
+ READ_ONCE(peer->dst_cache_mark) == mark)
+ dst_cache_set_ip4(cache, &rt->dst, fl.saddr);
+ else
+ dst_cache_reset(cache);
transmit:
udp_tunnel_xmit_skb(rt, sk, skb, fl.saddr, fl.daddr, 0,
@@ -221,24 +242,30 @@ static int ovpn_udp4_output(struct ovpn_peer *peer,
struct ovpn_bind *bind,
*/
static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
struct dst_cache *cache, struct sock *sk,
- struct sk_buff *skb)
+ struct sk_buff *skb, __be16 sport, u32 mark)
{
struct dst_entry *dst;
+ struct in6_addr local, saddr;
int ret;
struct flowi6 fl = {
.daddr = bind->remote.in6.sin6_addr,
- .fl6_sport = inet_sk(sk)->inet_sport,
+ .fl6_sport = sport,
.fl6_dport = bind->remote.in6.sin6_port,
.flowi6_proto = sk->sk_protocol,
- .flowi6_mark = sk->sk_mark,
+ .flowi6_mark = mark,
.flowi6_oif = bind->remote.in6.sin6_scope_id,
};
/* bind->local is updated in place under peer->lock; read the 128-bit
- * address under the peer seqcount to avoid a torn read
+ * address under the peer seqcount to avoid a torn read. Snapshot it so
+ * the post-lookup validity check can compare against the value we
+ * actually resolved with: fl.saddr may be overwritten by the FIB when
+ * the local address is unset, and comparing bind->local against that
+ * would spuriously skip caching.
*/
ovpn_peer_local_ipv6(peer, bind, &fl.saddr);
+ saddr = fl.saddr;
local_bh_disable();
dst = dst_cache_get_ip6(cache, &fl.saddr);
@@ -251,6 +278,7 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct
ovpn_bind *bind,
* new look up
*/
fl.saddr = in6addr_any;
+ saddr = in6addr_any;
spin_lock_bh(&peer->lock);
write_seqcount_begin(&peer->bind_local_seq);
bind->local.ipv6 = in6addr_any;
@@ -267,7 +295,22 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct
ovpn_bind *bind,
&bind->remote.in6, ret);
goto err;
}
- dst_cache_set_ip6(cache, dst, &fl.saddr);
+ /* only cache the result if the parameters we resolved with are still
+ * current: a concurrent ovpn_peer_endpoints_update() may have replaced
+ * the bind (float) or updated bind->local in place, and a concurrent
+ * ovpn_udp_output() may have re-tagged the cache with a different
+ * sport/mark after we sampled them. In any of these cases the cache was
+ * already reset and re-caching a route resolved with the stale values
+ * would poison it, so bail out and reset instead.
+ */
+ ovpn_peer_local_ipv6(peer, bind, &local);
+ if (rcu_access_pointer(peer->bind) == bind &&
+ ipv6_addr_equal(&local, &saddr) &&
+ READ_ONCE(peer->dst_cache_sport) == sport &&
+ READ_ONCE(peer->dst_cache_mark) == mark)
+ dst_cache_set_ip6(cache, dst, &fl.saddr);
+ else
+ dst_cache_reset(cache);
transmit:
/* user IPv6 packets may be larger than the transport interface
@@ -306,12 +349,40 @@ static int ovpn_udp_output(struct ovpn_peer *peer, struct
dst_cache *cache,
struct sock *sk, struct sk_buff *skb)
{
struct ovpn_bind *bind;
+ __be16 sport;
+ u32 mark;
int ret;
/* set sk to null if skb is already orphaned */
if (!skb->destructor)
skb->sk = NULL;
+ sport = READ_ONCE(inet_sk(sk)->inet_sport);
+ if (unlikely(!sport)) {
+ /* the transport UDP socket has been disconnected (e.g. via
+ * connect(AF_UNSPEC)): inet_sport == 0 means the socket has
+ * been unhashed and sending from source port 0 is nonsense;
+ * refuse and tell the operator
+ */
+ netdev_warn_once(peer->ovpn->dev,
+ "UDP transport socket has no source port; was
it disconnected?\n");
+ return -EIO;
+ }
+ mark = READ_ONCE(sk->sk_mark);
+
+ /* userspace can change sk_mark (via setsockopt(SO_MARK)) and
+ * inet_sport (via connect(AF_UNSPEC)) at any time without notifying
+ * ovpn; if either differs from what the dst_cache was last populated
+ * with, invalidate the cache now so a hit doesn't return a dst
+ * resolved with the old value
+ */
+ if (READ_ONCE(peer->dst_cache_sport) != sport ||
+ READ_ONCE(peer->dst_cache_mark) != mark) {
+ dst_cache_reset(cache);
+ WRITE_ONCE(peer->dst_cache_sport, sport);
+ WRITE_ONCE(peer->dst_cache_mark, mark);
+ }
+
rcu_read_lock();
bind = rcu_dereference(peer->bind);
if (unlikely(!bind)) {
@@ -323,11 +394,11 @@ static int ovpn_udp_output(struct ovpn_peer *peer, struct
dst_cache *cache,
switch (bind->remote.in4.sin_family) {
case AF_INET:
- ret = ovpn_udp4_output(peer, bind, cache, sk, skb);
+ ret = ovpn_udp4_output(peer, bind, cache, sk, skb, sport, mark);
break;
#if IS_ENABLED(CONFIG_IPV6)
case AF_INET6:
- ret = ovpn_udp6_output(peer, bind, cache, sk, skb);
+ ret = ovpn_udp6_output(peer, bind, cache, sk, skb, sport, mark);
break;
#endif
default:
--
2.54.0
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
