linux-kernel - Re: [PATCH net-next v11 15/23] ovpn: implement keepalive mechanism

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: <df0e2597-1f7b-4a72-b607-ab2cbd03c79b@openvpn.net>
Date: Sun, 24 Nov 2024 01:28:07 +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 15/23] ovpn: implement keepalive mechanism

On 22/11/2024 17:18, Sabrina Dubroca wrote:
> 2024-11-22, 10:41:26 +0100, Antonio Quartulli wrote:
>> On 12/11/2024 14:20, Antonio Quartulli wrote:
>> [...]
>>>>> +static int ovpn_peer_del_nolock(struct ovpn_peer *peer,
>>>>> +                enum ovpn_del_peer_reason reason)
>>>>> +{
>>>>> +    switch (peer->ovpn->mode) {
>>>>> +    case OVPN_MODE_MP:
>>>>
>>>> I think it would be nice to add
>>>>
>>>>       lockdep_assert_held(&peer->ovpn->peers->lock);
>>
>> Sabrina, in other places I have used the sparse notation __must_hold()
>> instead.
>> Is there any preference in regards to lockdep vs sparse?
>>
>> I could switch them all to lockdep_assert_held if needed.
> 
> __must_hold has the advantage of being checked at compile time (though
> I'm not sure it's that reliable [1]), so you don't need to run a test
> that actually hits that particular code path.
> 
> In this case I see lockdep_assert_held as mainly documenting that the
> locking that makes ovpn_peer_del_nolock safe (as safe as
> ovpn_peer_del) is provided by its caller. The splat for incorrect use
> on debug kernels is a bonus. Sprinkling lockdep_assert_held all over
> ovpn might be bloating the code too much, but I'm not opposed to
> adding them if it helps.
> 
> [1] I ran sparse on drivers/net/ovpn/peer.c before/after removing the
> locking from ovpn_peer_del and didn't get any warnings. sparse is good
> to detect imbalances (function that locks without unlocking), but
> maybe don't trust __must_hold for more than documenting expectations.

Same here. I didn't expect that.
Then I think it's better to rely on lockdep_assert_held() for this kind 
of assumptions.

> 
> [note: if you end up merging ovpn->peers->lock with ovpn->lock as
> we've discussed somewhere else, the locking around keepalive and
> ovpn_peer_del becomes a bit less hairy]

Yeah, this is happening.

Thanks a lot!

Regards,

> 

-- 
Antonio Quartulli
OpenVPN Inc.