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

[Sabrina Dubroca <sd@queasysnail.net>]

2024-05-29, 22:15:27 +0200, Antonio Quartulli wrote:
> On 29/05/2024 17:16, Sabrina Dubroca wrote:
> > 2024-05-28, 21:41:15 +0200, Antonio Quartulli wrote:
> > > 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.
> > 
> > Cool :)
> > 
> > > > 2024-05-06, 03:16:27 +0200, Antonio Quartulli wrote:
> > > > > +	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.
> > 
> > hlist_del_init_rcu does nothing if node is not already on a list.
> 
> Mh you're right. I must have got confused for some reason.
> Those del_init_rcu can go then.
> 
> > 
> > > While hash_entry_id always goes through hlist_add_head_rcu, therefore
> > > del_init_rcu is useless (to my understanding).
> > 
> > I'm probably missing something about how this all fits together. In
> > patch 19, I see ovpn_nl_set_peer_doit can re-add a peer that is
> > already added (but I'm not sure why, since you don't allow changing
> > the addresses, so it won't actually be re-hashed).
> 
> Actually it's not a "re-add", but the intent is to "update" a peer that
> already exists. However, some fields are forbidden from being updated, like
> the address.
> 
> [NOTE: I found some issue with the "peer update" logic in
> ovpn_nl_set_peer_doit and it's being changed a bit]
> 
> > 
> > I don't think doing a 2nd add of the same element to peers.by_id (or
> > any of the other hashtables) is correct, so I'd say you need
> > hlist_del_init_rcu for all of them.
> 
> This is exactly the bug I mentioned above: we should not go through the add
> again. Ideally we should just update the fields and be done with it, without
> re-hashing the object.

Ok, if you only call ovpn_peer_add for new peers, this looks fine and
the hlist_del_init_rcu can all be removed as you said.

Thanks.

-- 
Sabrina