netdev - Re: [PATCH net-next 2/4] ovpn: ensure sk is still valid during cleanup

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] [day] [month] [year] [list]

Message-ID: <736d1f6d-1b07-4671-857d-a0841828fce1@openvpn.net>
Date: Thu, 29 May 2025 12:07:48 +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>,
 Oleksandr Natalenko <oleksandr@...alenko.name>,
 Qingfang Deng <dqfext@...il.com>, Gert Doering <gert@...enie.muc.de>
Subject: Re: [PATCH net-next 2/4] ovpn: ensure sk is still valid during
 cleanup

On 29/05/2025 11:44, Paolo Abeni wrote:
> On 5/27/25 3:46 PM, Antonio Quartulli wrote:
>> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
>> index bea03913bfb1..1dd7e763c168 100644
>> --- a/drivers/net/ovpn/netlink.c
>> +++ b/drivers/net/ovpn/netlink.c
>> @@ -423,9 +423,14 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>>   	ovpn_sock = ovpn_socket_new(sock, peer);
>>   	/* at this point we unconditionally drop the reference to the socket:
>>   	 * - in case of error, the socket has to be dropped
>> -	 * - if case of success, the socket is configured and let
>> +	 * - if case of success, the socket is configured and we let
>>   	 *   userspace own the reference, so that the latter can
>> -	 *   trigger the final close()
>> +	 *   trigger the final close().
>> +	 *
>> +	 * NOTE: at this point ovpn_socket_new() has acquired a reference
>> +	 * to sock->sk. That's needed especially to avoid race conditions
>> +	 * during cleanup, where sock may still be alive, but sock->sk may be
>> +	 * getting released concurrently.
> 
> This comment duplicate some wording from commit message contents and
> don't add much value IMHO. It could be dropped.

ACK

> 
>> @@ -192,19 +189,30 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
>>   		rcu_read_unlock();
>>   	}
>>   
>> +	/* increase sk refcounter as we'll store a reference in
>> +	 * ovpn_socket.
>> +	 * ovpn_socket_release() will decrement the refcounter.
>> +	 */
>> +	if (!refcount_inc_not_zero(&sk->sk_refcnt)) {
> 
> How could sk_refcnt be zero here? likely just sock_hold() is sufficient.
> Also I think the reference could be acquired a little later, avoiding at
> least the following chunk.

I just wanted to be safe.
But as you point out I don't see any reason why sk_refcnt would be 0 at 
this point in time.

If we don't have to check the return value, I can definitely postpone 
the sock_hold() a bit more.

> 
> Also IMHO the comment is not very clear. I think it should state
> explicitly which entity is retaining the reference (AFAICS the peer hash
> table).

Ok, I will elaborate a bit more.
Essentially: sk -> ovpn_socket -> ovpn_peer -> peer hash

Thanks a lot!
Regards,

> 
> /P
> 

-- 
Antonio Quartulli
OpenVPN Inc.