linux-kernel - Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZzSH-Ke4wuJcis0q@hog>
Date: Wed, 13 Nov 2024 12:05:28 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Donald Hunter <donald.hunter@...il.com>,
	Shuah Khan <shuah@...nel.org>, ryazanov.s.a@...il.com,
	Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 18/23] ovpn: implement peer
 add/get/dump/delete via netlink

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