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

Antonio Quartulli
2024-05-28T19:39:48Z

On 28/05/2024 16:44, Sabrina Dubroca wrote:
> Hi Antonio, I took a little break but I'm looking at your patches
> again now.

Thanks Sabrina! Meanwhile I have been working on all your suggested changes.
Right now I am familiarizing with the strparser.

>
> 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.

Right - for clients it doesn't make sense.

>
> 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.

Yeah, makes sense. I'll allocate it dynamically then.

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

ACK

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

I think not calling del_init_rcu on hash_entry_addr4 was a mistake.

Calling del_init_rcu on addr4, addr6 and transp_addr is needed to put
them in a known state in case they are not hashed.

While hash_entry_id always goes through hlist_add_head_rcu, therefore
del_init_rcu is useless (to my understanding).

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

ACK

>
>> + 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]);
>> + }
>> +
>

--
Antonio Quartulli
OpenVPN Inc.