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: <5ae6f624-5196-42f7-a0b8-85e2847b3fdf@openvpn.net>
Date: Thu, 14 Nov 2024 11:32:36 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.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

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.