On 29/05/2024 17:16, Sabrina Dubroca wrote:

2024-05-28, 21:41:15 +0200, Antonio Quartulli wrote:

quoted

On 28/05/2024 16:44, Sabrina Dubroca wrote:

quoted

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

quoted

quoted

2024-05-06, 03:16:27 +0200, Antonio Quartulli wrote:

quoted

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

quoted

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.

I hope it makes sense.

Cheers,

-- 
Antonio Quartulli
OpenVPN Inc.