Re: [PATCH net-next v3 14/24] ovpn: implement multi-peer support

[Sabrina Dubroca <sd@queasysnail.net>]

Hi Antonio, I took a little break but I'm looking at your patches
again now.

2024-05-06, 03:16:27 +0200, Antonio Quartulli wrote:
> diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ovpnstruct.h
> index 7414c2459fb9..58166fdeac63 100644
> --- a/drivers/net/ovpn/ovpnstruct.h
> +++ b/drivers/net/ovpn/ovpnstruct.h
> @@ -31,6 +35,12 @@ struct ovpn_struct {
>  	spinlock_t lock; /* protect writing to the ovpn_struct object */
>  	struct workqueue_struct *crypto_wq;
>  	struct workqueue_struct *events_wq;
> +	struct {
> +		DECLARE_HASHTABLE(by_id, 12);
> +		DECLARE_HASHTABLE(by_transp_addr, 12);
> +		DECLARE_HASHTABLE(by_vpn_addr, 12);

Those are really big. I guess for large servers they make sense, but
you're making clients hold 98kB in memory that they're not going to use.

Maybe they could be dynamically sized, but I think struct peers should
be allocated on demand (only for mode == MP) if you want this size.

> +		spinlock_t lock; /* protects writes to peers tables */
> +	} peers;
>  	struct ovpn_peer __rcu *peer;
>  	struct list_head dev_list;
>  };
> diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
> index 99a2ae42a332..38a89595dade 100644
> --- a/drivers/net/ovpn/peer.c
> +++ b/drivers/net/ovpn/peer.c
> @@ -361,6 +362,91 @@ struct ovpn_peer *ovpn_peer_get_by_src(struct ovpn_struct *ovpn,
>  	return peer;
>  }
>  
> +/**
> + * ovpn_peer_add_mp - add per to related tables in a MP instance
                             ^
                             s/per/peer/

> + * @ovpn: the instance to add the peer to
> + * @peer: the peer to add
> + *
> + * Return: 0 on success or a negative error code otherwise
> + */
> +static int ovpn_peer_add_mp(struct ovpn_struct *ovpn, struct ovpn_peer *peer)
> +{
[...]
> +	index = ovpn_peer_index(ovpn->peers.by_id, &peer->id, sizeof(peer->id));
> +	hlist_add_head_rcu(&peer->hash_entry_id, &ovpn->peers.by_id[index]);
> +
> +	if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) {
> +		index = ovpn_peer_index(ovpn->peers.by_vpn_addr,
> +					&peer->vpn_addrs.ipv4,
> +					sizeof(peer->vpn_addrs.ipv4));
> +		hlist_add_head_rcu(&peer->hash_entry_addr4,
> +				   &ovpn->peers.by_vpn_addr[index]);
> +	}
> +
> +	hlist_del_init_rcu(&peer->hash_entry_addr6);

Why are hash_entry_transp_addr and hash_entry_addr6 getting a
hlist_del_init_rcu() call, but not hash_entry_id and hash_entry_addr4?

> +	if (memcmp(&peer->vpn_addrs.ipv6, &in6addr_any,
> +		   sizeof(peer->vpn_addrs.ipv6))) {

!ipv6_addr_any(&peer->vpn_addrs.ipv6)

> +		index = ovpn_peer_index(ovpn->peers.by_vpn_addr,
> +					&peer->vpn_addrs.ipv6,
> +					sizeof(peer->vpn_addrs.ipv6));
> +		hlist_add_head_rcu(&peer->hash_entry_addr6,
> +				   &ovpn->peers.by_vpn_addr[index]);
> +	}
> +

-- 
Sabrina