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

Sabrina Dubroca
2024-05-28T14:44:36Z

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