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

[Antonio Quartulli]

On 03/03/2025 14:08, Sabrina Dubroca wrote:
> 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)

Yeah, makes sense!

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

Ouch, good catch.

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

Hehe, it's like a double negation. I agree it can be improved.

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

does the netdev coding style allow to use locally scoped variables?
Or should I declare everything at the beginning of the function?

I had this rule in mind, but it may have been eliminated by now.

> 		/* 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)

Yours look simpler/cleaner. I'll go with it.

Thanks!

Cheers,

>

--
Antonio Quartulli
OpenVPN Inc.