Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object

[Antonio Quartulli <antonio@openvpn.net>]

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.