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

Sabrina Dubroca
2024-05-29T15:17:18Z

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