On 10/05/2024 15:01, Sabrina Dubroca wrote:

2024-05-06, 03:16:22 +0200, Antonio Quartulli wrote:

quoted

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index a420bb45f25f..36cfb95edbf4 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c

@@ -28,6 +30,12 @@ int ovpn_struct_init(struct net_device *dev)
  
  	spin_lock_init(&ovpn->lock);
  
+	ovpn->crypto_wq = alloc_workqueue("ovpn-crypto-wq-%s",
+					  WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM, 0,
+					  dev->name);
+	if (!ovpn->crypto_wq)
+		return -ENOMEM;
+
  	ovpn->events_wq = alloc_workqueue("ovpn-events-wq-%s", WQ_MEM_RECLAIM,
  					  0, dev->name);
  	if (!ovpn->events_wq)
  		return -ENOMEM;

This will leak crypto_wq on failure. You need to roll back all
previous changes when something fails (also if you move all this stuff
into ndo_init).

ouch, good catch! thanks.

quoted

diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index 659df320525c..f915afa260c3 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h

@@ -22,9 +23,12 @@
   * @id: unique identifier
   * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel
   * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel
+ * @encrypt_work: work used to process outgoing packets
+ * @decrypt_work: work used to process incoming packets

nit: Only encrypt_work is used in this patch, decrypt_work is for RX

Right, same for tx_ring actually. will move both to the next patch

quoted

diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index 4b7d96a13df0..f434da76dc0a 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
+/**
+ * ovpn_udp4_output - send IPv4 packet over udp socket
+ * @ovpn: the openvpn instance
+ * @bind: the binding related to the destination peer
+ * @cache: dst cache
+ * @sk: the socket to send the packet over
+ * @skb: the packet to send
+ *
+ * Return: 0 on success or a negative error code otherwise
+ */
+static int ovpn_udp4_output(struct ovpn_struct *ovpn, struct ovpn_bind *bind,
+			    struct dst_cache *cache, struct sock *sk,
+			    struct sk_buff *skb)
+{
+	struct rtable *rt;
+	struct flowi4 fl = {
+		.saddr = bind->local.ipv4.s_addr,
+		.daddr = bind->sa.in4.sin_addr.s_addr,
+		.fl4_sport = inet_sk(sk)->inet_sport,
+		.fl4_dport = bind->sa.in4.sin_port,
+		.flowi4_proto = sk->sk_protocol,
+		.flowi4_mark = sk->sk_mark,
+	};
+	int ret;
+
+	local_bh_disable();
+	rt = dst_cache_get_ip4(cache, &fl.saddr);
+	if (rt)
+		goto transmit;
+
+	if (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

What exactly are you trying to guard against here? The ipv4 address
used for the last packet being removed from the device/host? I don't
see other tunnels using dst_cache doing this kind of thing (except
wireguard).

yes, that's the scenario being checked (which hopefully is what the 
comment conveys).

quoted

+		 */
+		fl.saddr = 0;
+		bind->local.ipv4.s_addr = 0;
+		dst_cache_reset(cache);
+	}
+
+	rt = ip_route_output_flow(sock_net(sk), &fl, sk);
+	if (IS_ERR(rt) && PTR_ERR(rt) == -EINVAL) {
+		fl.saddr = 0;
+		bind->local.ipv4.s_addr = 0;
+		dst_cache_reset(cache);
+
+		rt = ip_route_output_flow(sock_net(sk), &fl, sk);

Why do you need to repeat the lookup? And why only for ipv4, but not
for ipv6?

We are repeating the lookup without the saddr.

The first lookup may have failed because the destination is not 
reachable anymore from that specific source address, but it may be 
reachable from another one (i.e. routing table change in a multi-homed 
setup).

Why not for v6..that's a good question..I wonder if I should just do the 
same and repeat the lookup with ip6addr_any as source..I think it would 
make sense as we could end up in the same scenario as described for IPv4.

What do you think?

quoted

+	}
+
+	if (IS_ERR(rt)) {
+		ret = PTR_ERR(rt);
+		net_dbg_ratelimited("%s: no route to host %pISpc: %d\n",
+				    ovpn->dev->name, &bind->sa.in4, ret);
+		goto err;
+	}
+	dst_cache_set_ip4(cache, &rt->dst, fl.saddr);

Overall this looks a whole lot like udp_tunnel_dst_lookup, except for:
  - 2nd lookup
  - inet_confirm_addr/dst_cache_reset

but why doesn't udp_tunnel_dst_lookup account for cases where the source 
address is not usable anymore? I think they are reasonable, no?

Maybe I could still use udp_tunnel_dst_lookup, but call it a second time 
without saddr in case of failure?

(and there's udp_tunnel6_dst_lookup for ipv6)

-- 
Antonio Quartulli
OpenVPN Inc.