netdev - Re: [PATCH net-next v5 17/25] 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: <1554a7e8-026c-42cf-8938-1de38171b048@openvpn.net>
Date: Thu, 18 Jul 2024 10:22:11 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, ryazanov.s.a@...il.com,
 pabeni@...hat.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v5 17/25] ovpn: implement keepalive mechanism

On 17/07/2024 22:40, Sabrina Dubroca wrote:
>>>> +/**
>>>> + * ovpn_peer_keepalive_recv_reset - reset keepalive timeout
>>>> + * @peer: peer for which the timeout should be reset
>>>> + *
>>>> + * To be invoked upon reception of an authenticated packet from peer in order
>>>> + * to report valid activity and thus reset the keepalive timeout
>>>> + */
>>>> +static inline void ovpn_peer_keepalive_recv_reset(struct ovpn_peer *peer)
>>>> +{
>>>> +	u32 delta = msecs_to_jiffies(peer->keepalive_timeout * MSEC_PER_SEC);
>>>> +
>>>> +	if (unlikely(!delta))
>>>> +		return;
>>>> +
>>>> +	mod_timer(&peer->keepalive_recv, jiffies + delta);
>>>
>>> This (and ovpn_peer_keepalive_xmit_reset) is going to be called for
>>> each packet. I wonder how well the timer subsystem deals with one
>>> timer getting updated possibly thousands of time per second.
>>>
>>
>> May it even introduce some performance penalty?
> 
> That's what I was worried about, yes.
> 
> I asked Paolo, he suggested checking that we're actually doing any
> change to the timer:
> 
>     if (new_timeout_time != old_timeout_time)
>         mod_timer(...)
> 
> This would reduce the update frequency to one per jiffy, which should
> be acceptable.
> 
>> Maybe we should get rid of the timer object and introduce a periodic (1s)
>> worker which checks some last_recv timestamp on every known peer?
>> What do you think?
> 
> That should work, or the workqueue like Eyal is saying.

I will go with Eyal's approach.
This way we also eliminate any timer related operation from the fast path.


Cheers!



-- 
Antonio Quartulli
OpenVPN Inc.