linux-kernel - Re: [PATCH net-next v20 15/25] ovpn: implement multi-peer support

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: <Z8WpxDpHYzG9pXNl@hog>
Date: Mon, 3 Mar 2025 14:08:20 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, 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+netdev@...n.ch>,
	Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH net-next v20 15/25] ovpn: implement multi-peer support

Hello, a few minor coding style nits on this patch.

2025-02-27, 02:21:40 +0100, Antonio Quartulli wrote:
> @@ -197,9 +254,16 @@ static int ovpn_netdev_notifier_call(struct notifier_block *nb,
>  		netif_carrier_off(dev);
>  		ovpn->registered = false;
>  
> -		if (ovpn->mode == OVPN_MODE_P2P)
> +		switch (ovpn->mode) {
> +		case OVPN_MODE_P2P:
>  			ovpn_peer_release_p2p(ovpn, NULL,
>  					      OVPN_DEL_PEER_REASON_TEARDOWN);
> +			break;
> +		case OVPN_MODE_MP:
> +			ovpn_peers_free(ovpn, NULL,
> +					OVPN_DEL_PEER_REASON_TEARDOWN);
> +			break;
> +		}

nit: maybe that switch could be done inside ovpn_peers_free, since
both places calling ovpn_peers_free do the same thing?
(it would also be more consistent with the rest of the peer-related
functions that are wrappers for the _mp/_p2p variant, rather than
pushing the switch down to the caller)


> +void ovpn_peers_free(struct ovpn_priv *ovpn, struct sock *sk,
> +		     enum ovpn_del_peer_reason reason)
> +{
> +	struct ovpn_socket *ovpn_sock;
> +	LLIST_HEAD(release_list);
> +	struct ovpn_peer *peer;
> +	struct hlist_node *tmp;
> +	bool skip;
> +	int bkt;
> +
> +	spin_lock_bh(&ovpn->lock);
> +	hash_for_each_safe(ovpn->peers->by_id, bkt, tmp, peer, hash_entry_id) {
> +		/* if a socket was passed as argument, skip all peers except
> +		 * those using it
> +		 */
> +		if (sk) {
> +			skip = true;
> +
> +			rcu_read_lock();
> +			ovpn_sock = rcu_access_pointer(peer->sock);

rcu_dereference, since you're actually accessing ovpn_sock->sock
afterwards?

> +			if (ovpn_sock && ovpn_sock->sock->sk == sk)
> +				skip = false;
> +			rcu_read_unlock();
> +
> +			if (skip)
> +				continue;


The skip/continue logic looks a tiny bit strange to me, maybe this:

	hash_for_each_safe(ovpn->peers->by_id, bkt, tmp, peer, hash_entry_id) {
		bool remove = true;

		/* if a socket was passed as argument, skip all peers except
		 * those using it
		 */
		if (sk) {
			rcu_read_lock();
			ovpn_sock = rcu_dereference(peer->sock);
			remove = ovpn_sock && ovpn_sock->sock->sk == sk;
			rcu_read_unlock();
		}

		if (remove)
			ovpn_peer_remove(peer, reason, &release_list);
	}


(only if you agree it looks better - if it's my opinion against yours,
ignore me since it's really just coding style/taste)

-- 
Sabrina