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: <4df15a91-4bcb-49d8-be78-28c71036ba8a@openvpn.net>
Date: Wed, 30 Oct 2024 21:47:58 +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 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.

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

Cheers,

> 
>> +
>> +		return -ENOENT;
>> +	}
>> +
>> +	tmp->delete_reason = reason;
>> +	RCU_INIT_POINTER(peer->ovpn->peer, NULL);
>> +	ovpn_peer_put(tmp);
>> +
>> +	return 0;
>> +}
> 

-- 
Antonio Quartulli
OpenVPN Inc.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ