Re: [PATCH net-next v3 09/24] ovpn: implement basic TX path (UDP)

Sabrina Dubroca
2024-05-10T13:02:00Z

2024-05-06, 03:16:22 +0200, Antonio Quartulli wrote:
> 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).

> 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

> 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