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

From: Sabrina Dubroca <[email-masked]>
Date: Wed, 29 May 2024 17:16:52 +0200

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.

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

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.

-- 
Sabrina