linux-kernel - Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn

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: <ea28f799-cd93-4b69-86b0-28bc03217d0c@openvpn.net>
Date: Tue, 12 Nov 2024 11:12:41 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, ryazanov.s.a@...il.com,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object

On 05/11/2024 14:12, Sabrina Dubroca wrote:
> 2024-10-30, 21:47:58 +0100, Antonio Quartulli wrote:
>> On 30/10/2024 17:37, Sabrina Dubroca wrote:
>>> 2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote:
>>>> +static void ovpn_peer_release(struct ovpn_peer *peer)
>>>> +{
>>>> +	ovpn_bind_reset(peer, NULL);
>>>> +
>>>> +	dst_cache_destroy(&peer->dst_cache);
>>>
>>> Is it safe to destroy the cache at this time? In the same function, we
>>> use rcu to free the peer, but AFAICT the dst_cache will be freed
>>> immediately:
>>>
>>> void dst_cache_destroy(struct dst_cache *dst_cache)
>>> {
>>> [...]
>>> 	free_percpu(dst_cache->cache);
>>> }
>>>
>>> (probably no real issue because ovpn_udp_send_skb gets called while we
>>> hold a reference to the peer?)
>>
>> Right.
>> That was my assumption: release happens on refcnt = 0 only, therefore no
>> field should be in use anymore.
>> Anything that may still be in use will have its own refcounter.
> 
> My worry is that code changes over time, assumptions are forgotten,
> and we end up with code that was a bit odd but safe not being safe
> anymore.

Yeah, makes sense.
I'll move the call to dst_cache_destroy() and to kfree(peer) in a RCU 
callback.

Thanks!

> 
>>>
>>>> +	netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
>>>> +	kfree_rcu(peer, rcu);
>>>> +}
>>>
>>>
>>> [...]
>>>> +static int ovpn_peer_del_p2p(struct ovpn_peer *peer,
>>>> +			     enum ovpn_del_peer_reason reason)
>>>> +	__must_hold(&peer->ovpn->lock)
>>>> +{
>>>> +	struct ovpn_peer *tmp;
>>>> +
>>>> +	tmp = rcu_dereference_protected(peer->ovpn->peer,
>>>> +					lockdep_is_held(&peer->ovpn->lock));
>>>> +	if (tmp != peer) {
>>>> +		DEBUG_NET_WARN_ON_ONCE(1);
>>>> +		if (tmp)
>>>> +			ovpn_peer_put(tmp);
>>>
>>> Does peer->ovpn->peer need to be set to NULL here as well? Or is it
>>> going to survive this _put?
>>
>> First of all consider that this is truly something that we don't expect to
>> happen (hence the WARN_ON).
>> If this is happening it's because we are trying to delete a peer that is not
>> the one we are connected to (unexplainable scenario in p2p mode).
>>
>> Still, should we hit this case (I truly can't see how), I'd say "leave
>> everything as is - maybe this call was just a mistake".
> 
> Yeah, true, let's leave it. Thanks.
> 

-- 
Antonio Quartulli
OpenVPN Inc.