[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bdcc035e-24c2-4469-a0fd-f63494d74169@openvpn.net>
Date: Wed, 26 Mar 2025 11:22:37 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Qingfang Deng <dqfext@...il.com>
Cc: Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org, netdev@...r.kernel.org,
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>, sd@...asysnail.net, ryazanov.s.a@...il.com,
Andrew Lunn <andrew+netdev@...n.ch>, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH net-next v24 09/23] ovpn: implement packet processing
On 26/03/2025 11:03, Qingfang Deng wrote:
> Hi Antonio,
>
> On Wed, Mar 26, 2025 at 5:41 PM Antonio Quartulli <antonio@...nvpn.net> wrote:
>>>> +/* Get the next packet ID for xmit */
>>>> +static inline int ovpn_pktid_xmit_next(struct ovpn_pktid_xmit *pid, u32 *pktid)
>>>> +{
>>>> + const s64 seq_num = atomic64_fetch_add_unless(&pid->seq_num, 1,
>>>> + 0x100000000LL);
>>>> + /* when the 32bit space is over, we return an error because the packet
>>>> + * ID is used to create the cipher IV and we do not want to reuse the
>>>> + * same value more than once
>>>> + */
>>>> + if (unlikely(seq_num == 0x100000000LL))
>>>> + return -ERANGE;
>>>
>>> You may use a 32-bit atomic_t, instead of checking if it equals
>>> 0x1_00000000, check if it wraparounds to zero.
>>> Additionally, you don't need full memory ordering as you just want an
>>> incrementing value:
>>>
>>> int seq_num = atomic_fetch_inc_relaxed(&pid->seq_num);
>>>
>>> if (unlikely(!seq_num))
>>> ...
>>
>> But then if we have concurrent invocations of ovpn_pktid_xmit_next()
>> only the first one will error out on wrap-around, while the others will
>> return no error (seq_num becomes > 0) and will allow the packets to go
>> through.
>>
>> This is not what we want.
>
> Got it. You could replace it with
> atomic_fetch_add_unless(&pid->seq_num, 1, 0) and check if it wraps
> around to zero.
What about the first time when seq_num is 0? It will already stop, no?
>
> However, what about the opposite scenario? If multiple threads
> concurrently invoke ovpn_pktid_xmit_next() and all detect the
> wraparound condition, could this lead to simultaneous calls to
> ovpn_crypto_kill_key() and ovpn_nl_key_swap_notify()?
Calling ovpn_crypto_kill_key() multiple times is not an issue, as only
the first time it'll do something. Subsequent calls are no-op.
But you're right about ovpn_nl_key_swap_notify(): each call will produce
a notification which we don't want.
I'll make it conditional so that we'll send a notification only if
ovpn_crypto_kill_key() did the killing.
Thanks for asking this!
Regards,
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists