netdev - Re: [PATCH net-next 1/4] ovpn: properly deconfigure UDP-tunnel

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: <40126ac9-f911-4216-ba69-5e5ef051dd63@openvpn.net>
Date: Thu, 29 May 2025 11:40:35 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc: Sabrina Dubroca <sd@...asysnail.net>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
 Oleksandr Natalenko <oleksandr@...alenko.name>
Subject: Re: [PATCH net-next 1/4] ovpn: properly deconfigure UDP-tunnel

Hi Paolo,

On 29/05/2025 11:25, Paolo Abeni wrote:
> On 5/27/25 3:46 PM, Antonio Quartulli wrote:
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index dde52b8050b8..9ffc4e0b1644 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -2900,7 +2900,7 @@ void udp_destroy_sock(struct sock *sk)
>>   			if (encap_destroy)
>>   				encap_destroy(sk);
>>   		}
>> -		if (udp_test_bit(ENCAP_ENABLED, sk)) {
>> +		if (udp_test_and_clear_bit(ENCAP_ENABLED, sk)) {
> 
> Nothing should use 'sk' after udp_destroy_sock(), no need to clear the
> bit (same in ipv6 code)

Ok, then I guess that in ovpn I need to check if SOCK_DEAD is set and, 
if so, just bail out.

> 
>>   			static_branch_dec(&udp_encap_needed_key);
>>   			udp_tunnel_cleanup_gro(sk);
>>   		}
>> diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
>> index 2326548997d3..624b6afcf812 100644
>> --- a/net/ipv4/udp_tunnel_core.c
>> +++ b/net/ipv4/udp_tunnel_core.c
>> @@ -98,6 +98,28 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
>>   }
>>   EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
>>   
>> +void cleanup_udp_tunnel_sock(struct sock *sk)
>> +{
>> +	/* Re-enable multicast loopback */
>> +	inet_set_bit(MC_LOOP, sk);
>> +
>> +	/* Disable CHECKSUM_UNNECESSARY to CHECKSUM_COMPLETE conversion */
>> +	inet_dec_convert_csum(sk);
>> +
>> +	udp_sk(sk)->encap_type = 0;
>> +	udp_sk(sk)->encap_rcv = NULL;
>> +	udp_sk(sk)->encap_err_rcv = NULL;
>> +	udp_sk(sk)->encap_err_lookup = NULL;
>> +	udp_sk(sk)->encap_destroy = NULL;
>> +	udp_sk(sk)->gro_receive = NULL;
>> +	udp_sk(sk)->gro_complete = NULL;
>> +
>> +	rcu_assign_sk_user_data(sk, NULL);
>> +	udp_tunnel_encap_disable(sk);
>> +	udp_tunnel_cleanup_gro(sk);
>> +}
>> +EXPORT_SYMBOL_GPL(cleanup_udp_tunnel_sock);
> 
> IMHO the above code should not land into a generic tunnel helper, as the
> tunnel scope always correspond to the sk scope - except for openvpn.
> 

Ok.
I thought udp_tunnel was the right place because this helper is an 
explicit counterpart for setup_udp_tunnel_sock().

But since there are no other users, I can move this code inside ovpn.

> Also IMHO no need to do udp_tunnel_encap_disable(), it will called as
> needed at sk close() time. Yep, that means that the stack will have
> transient additional overhead until the sock is not destroyed, but IMHO
> it's not worthy the extra complexity (export symbol, additional stub,
> more files touched...)

That's true: removing that part will greatly simplify the patch.

If we agree that we can live with the transient overhead, then ok, I 
will remove the udp_tunnel_encap_disable() part and simplify this commit.

Thanks for the feedback, Paolo!

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.