On 13/11/2024 12:05, Sabrina Dubroca wrote:
2024-11-12, 15:26:59 +0100, Antonio Quartulli wrote:
On 11/11/2024 16:41, Sabrina Dubroca wrote:
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
+void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer)
+ __must_hold(&peer->ovpn->peers->lock)
Changes to peer->vpn_addrs are not protected by peers->lock, so those
could be getting updated while we're rehashing (and taking peer->lock
in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent
that).
/me screams :-D
Sorry :)
Indeed peers->lock is only about protecting the lists, not the content of
the listed objects.
How about acquiring the peers->lock before calling ovpn_nl_peer_modify()?
It seems like it would work. Maybe a bit weird to have conditional
locking (MP mode only), but ok. You already have this lock ordering
(hold peers->lock before taking peer->lock) in
ovpn_peer_keepalive_work_mp, so there should be no deadlock from doing
the same thing in the netlink code.
Yeah.
Then I would also do that in ovpn_peer_float to protect that rehash.
I am not extremely comfortable with this, because it means acquiring
peers->lock on every packet (right now we do so only on peer->lock) and
it may defeat the advantage of the RCU locking on the hashtables.
Wouldn't you agree?
An alternative would be to hold peer->lock for the entire function, but
this will lead to dead locks...no go either.
It feels like peers->lock is turning into a duplicate of
ovpn->lock. ovpn->lock used for P2P mode, peers->lock used
equivalently for MP mode. You might consider merging them (but I
wouldn't see it as necessary for merging the series unless there's a
locking issue with the current proposal).
I agree: ovpn->lock was introduced to protect ovpn's fields, but
actually the only one e protect is peer.
They are truly the same and I could therefore get rid of
ovpn->peers->lock and always use ovpn->lock.
Will see how invasive this is and decide whether to commit it to v12 or not.
Thanks!
Regards,
--
Antonio Quartulli
OpenVPN Inc.
