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. Then I would also do that in ovpn_peer_float to protect that rehash. 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). -- Sabrina
