netdev - 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: <5eaf74fb-ad13-4371-95b8-7a5f1f3a9cda@openvpn.net>
Date: Fri, 22 Nov 2024 10:41:26 +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 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.

Regards,

>>
>>> +        return ovpn_peer_del_mp(peer, reason);
>>> +    case OVPN_MODE_P2P:
>>
>> and here
>>
>>      lockdep_assert_held(&peer->ovpn->lock);
> 
> Yeah, good idea.
> __must_hold() can't work here, so lockdep_assert_held is definitely the 
> way to go.
> 
>>
>> (I had to check that ovpn_peer_del_nolock is indeed called with those
>> locks held since they're taken by ovpn_peer_keepalive_work_{mp,p2p},
>> adding these assertions would make it clear that ovpn_peer_del_nolock
>> is not an unsafe version of ovpn_peer_del)
> 
> Right, it makes sense.
> 
>>
>>> +        return ovpn_peer_del_p2p(peer, reason);
>>> +    default:
>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +}
>>> +
>>>   /**
>>>    * ovpn_peers_free - free all peers in the instance
>>>    * @ovpn: the instance whose peers should be released
>>> @@ -830,3 +871,150 @@ void ovpn_peers_free(struct ovpn_struct *ovpn)
>>>           ovpn_peer_unhash(peer, OVPN_DEL_PEER_REASON_TEARDOWN);
>>>       spin_unlock_bh(&ovpn->peers->lock);
>>>   }
>>> +
>>> +static time64_t ovpn_peer_keepalive_work_single(struct ovpn_peer *peer,
>>> +                        time64_t now)
>>> +{
>>> +    time64_t next_run1, next_run2, delta;
>>> +    unsigned long timeout, interval;
>>> +    bool expired;
>>> +
>>> +    spin_lock_bh(&peer->lock);
>>> +    /* we expect both timers to be configured at the same time,
>>> +     * therefore bail out if either is not set
>>> +     */
>>> +    if (!peer->keepalive_timeout || !peer->keepalive_interval) {
>>> +        spin_unlock_bh(&peer->lock);
>>> +        return 0;
>>> +    }
>>> +
>>> +    /* check for peer timeout */
>>> +    expired = false;
>>> +    timeout = peer->keepalive_timeout;
>>> +    delta = now - peer->last_recv;
>>
>> I'm not sure that's always > 0 if we finish decrypting a packet just
>> as the workqueue starts:
>>
>>    ovpn_peer_keepalive_work
>>      now = ...
>>
>>                                         ovpn_decrypt_post
>>                                           peer->last_recv = ...
>>
>>    ovpn_peer_keepalive_work_single
>>      delta: now < peer->last_recv
>>
> 
> Yeah, there is nothing preventing this from happening...but is this 
> truly a problem? The math should still work, no?
> 
> However:
> 
>>
>>
>>> +    if (delta < timeout) {
>>> +        peer->keepalive_recv_exp = now + timeout - delta;
>>
>> I'd shorten that to
>>
>>      peer->keepalive_recv_exp = peer->last_recv + timeout;
>>
>> it's a bit more readable to my eyes and avoids risks of wrapping
>> values.
>>
>> So I'd probably get rid of delta and go with:
>>
>>      last_recv = READ_ONCE(peer->last_recv)
>>      if (now < last_recv + timeout) {
>>          peer->keepalive_recv_exp = last_recv + timeout;
>>          next_run1 = peer->keepalive_recv_exp;
>>      } else if ...
>>
>>> +        next_run1 = peer->keepalive_recv_exp;
>>> +    } else if (peer->keepalive_recv_exp > now) {
>>> +        next_run1 = peer->keepalive_recv_exp;
>>> +    } else {
>>> +        expired = true;
>>> +    }
> 
> I agree this is simpler to read and gets rid of some extra operations.
> 
> [note: I took inspiration from nat_keepalive_work_single() - it could be 
> simplified as well I guess]
> 
>>
>> [...]
>>> +    /* check for peer keepalive */
>>> +    expired = false;
>>> +    interval = peer->keepalive_interval;
>>> +    delta = now - peer->last_sent;
>>> +    if (delta < interval) {
>>> +        peer->keepalive_xmit_exp = now + interval - delta;
>>> +        next_run2 = peer->keepalive_xmit_exp;
>>
>> and same here
> 
> Yeah, will change both. Thanks!
> 
> 
> Regards,
> 
> 

-- 
Antonio Quartulli
OpenVPN Inc.