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

[Antonio Quartulli <antonio@openvpn.net>]

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.