On 26/03/2025 11:03, Qingfang Deng wrote:

Hi Antonio,

On Wed, Mar 26, 2025 at 5:41 PM Antonio Quartulli [off-list ref] wrote:

quoted

quoted

quoted

+/* 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.