lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fdc23343-a85c-41d3-ab24-51c3d4e78854@openvpn.net>
Date: Fri, 10 May 2024 15:39:33 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Sergey Ryazanov <ryazanov.s.a@...il.com>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, Andrew Lunn <andrew@...n.ch>,
 Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 09/24] ovpn: implement basic TX path (UDP)

On 10/05/2024 15:01, Sabrina Dubroca wrote:
> 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).

ouch, good catch! thanks.

> 
>> 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

> 
> 
>> 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).

> 
>> +		 */
>> +		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?

> 
>> +	}
>> +
>> +	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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ