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

[Sabrina Dubroca <sd@queasysnail.net>]

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