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

[Sabrina Dubroca <sd@queasysnail.net>]

2024-08-27, 14:07:55 +0200, Antonio Quartulli wrote:
>  static int ovpn_net_init(struct net_device *dev)
>  {
>  	struct ovpn_struct *ovpn = netdev_priv(dev);
> +	int i, err = gro_cells_init(&ovpn->gro_cells, dev);

I'm not a fan of "hiding" the gro_cells_init call up here. I'd prefer
if this was done just before the corresponding "if (err)".

> +	struct in_device *dev_v4;
>  
> -	return gro_cells_init(&ovpn->gro_cells, dev);
> +	if (err)
> +		return err;
> +
> +	if (ovpn->mode == OVPN_MODE_MP) {
> +		dev_v4 = __in_dev_get_rtnl(dev);
> +		if (dev_v4) {
> +			/* disable redirects as Linux gets confused by ovpn
> +			 * handling same-LAN routing.
> +			 * This happens because a multipeer interface is used as
> +			 * relay point between hosts in the same subnet, while
> +			 * in a classic LAN this would not be needed because the
> +			 * two hosts would be able to talk directly.
> +			 */
> +			IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
> +			IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;
> +		}
> +
> +		/* the peer container is fairly large, therefore we dynamically
> +		 * allocate it only when needed
> +		 */
> +		ovpn->peers = kzalloc(sizeof(*ovpn->peers), GFP_KERNEL);
> +		if (!ovpn->peers)

missing gro_cells_destroy

> +			return -ENOMEM;
> +
> +		spin_lock_init(&ovpn->peers->lock_by_id);
> +		spin_lock_init(&ovpn->peers->lock_by_vpn_addr);
> +		spin_lock_init(&ovpn->peers->lock_by_transp_addr);

What's the benefit of having 3 separate locks instead of a single lock
protecting all the hashtables?

> +
> +		for (i = 0; i < ARRAY_SIZE(ovpn->peers->by_id); i++) {
> +			INIT_HLIST_HEAD(&ovpn->peers->by_id[i]);
> +			INIT_HLIST_HEAD(&ovpn->peers->by_vpn_addr[i]);
> +			INIT_HLIST_NULLS_HEAD(&ovpn->peers->by_transp_addr[i],
> +					      i);
> +		}
> +	}
> +
> +	return 0;
>  }

> +static int ovpn_peer_add_mp(struct ovpn_struct *ovpn, struct ovpn_peer *peer)
> +{
> +	struct sockaddr_storage sa = { 0 };
> +	struct hlist_nulls_head *nhead;
> +	struct sockaddr_in6 *sa6;
> +	struct sockaddr_in *sa4;
> +	struct hlist_head *head;
> +	struct ovpn_bind *bind;
> +	struct ovpn_peer *tmp;
> +	size_t salen;
> +
> +	spin_lock_bh(&ovpn->peers->lock_by_id);
> +	/* do not add duplicates */
> +	tmp = ovpn_peer_get_by_id(ovpn, peer->id);
> +	if (tmp) {
> +		ovpn_peer_put(tmp);
> +		spin_unlock_bh(&ovpn->peers->lock_by_id);
> +		return -EEXIST;
> +	}
> +
> +	hlist_add_head_rcu(&peer->hash_entry_id,
> +			   ovpn_get_hash_head(ovpn->peers->by_id, &peer->id,
> +					      sizeof(peer->id)));
> +	spin_unlock_bh(&ovpn->peers->lock_by_id);
> +
> +	bind = rcu_dereference_protected(peer->bind, true);
> +	/* peers connected via TCP have bind == NULL */
> +	if (bind) {
> +		switch (bind->remote.in4.sin_family) {
> +		case AF_INET:
> +			sa4 = (struct sockaddr_in *)&sa;
> +
> +			sa4->sin_family = AF_INET;
> +			sa4->sin_addr.s_addr = bind->remote.in4.sin_addr.s_addr;
> +			sa4->sin_port = bind->remote.in4.sin_port;
> +			salen = sizeof(*sa4);
> +			break;
> +		case AF_INET6:
> +			sa6 = (struct sockaddr_in6 *)&sa;
> +
> +			sa6->sin6_family = AF_INET6;
> +			sa6->sin6_addr = bind->remote.in6.sin6_addr;
> +			sa6->sin6_port = bind->remote.in6.sin6_port;
> +			salen = sizeof(*sa6);
> +			break;
> +		default:

And remove from the by_id hashtable? Or is that handled somewhere that
I missed (I don't think ovpn_peer_unhash gets called in that case)?

> +			return -EPROTONOSUPPORT;
> +		}
> +

-- 
Sabrina