Re: [PATCH net-next v3 09/24] ovpn: implement basic TX path (UDP)
From: Sabrina Dubroca <sd@queasysnail.net>
Date: 2024-05-10 13:02:00
2024-05-06, 03:16:22 +0200, Antonio Quartulli wrote:
quoted hunk ↗ jump to hunk
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).
quoted hunk ↗ jump to hunk
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
quoted hunk ↗ jump to hunk
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).
+ */
+ 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?
+ }
+
+ 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 (and there's udp_tunnel6_dst_lookup for ipv6) -- Sabrina