netdev - 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: <ZyJgs6Vrvzji8qvS@hog>
Date: Wed, 30 Oct 2024 17:37:07 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.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

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?)

> +	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?

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

-- 
Sabrina