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

[Antonio Quartulli <antonio@openvpn.net>]

On 13/11/2024 11:36, Sabrina Dubroca wrote:
> 2024-11-12, 14:20:45 +0100, Antonio Quartulli wrote:
>> On 05/11/2024 19:10, Sabrina Dubroca wrote:
>>> 2024-10-29, 11:47:28 +0100, Antonio Quartulli wrote:
>>>> @@ -105,6 +132,9 @@ void ovpn_decrypt_post(void *data, int ret)
>>>>    		goto drop;
>>>>    	}
>>>> +	/* keep track of last received authenticated packet for keepalive */
>>>> +	peer->last_recv = ktime_get_real_seconds();
>>>
>>> It doesn't look like we're locking the peer here so that should be a
>>> WRITE_ONCE() (and READ_ONCE(peer->last_recv) for all reads).
>>
>> Is that because last_recv is 64 bit long (and might be more than one word on
>> certain architectures)?
>>
>> I don't remember having to do so for reading/writing 32 bit long integers.
> 
> AFAIK it's not just that. The compiler is free to do the read/write in
> any way it wants when you don't specify _ONCE. On the read side, it
> could read from memory a single time or multiple times (getting
> possibly different values each time), or maybe split the load
> (possibly reading chunks from different values being written in
> parallel).

Ok, thanks. Will switch to WRITE/READ_ONE then.

> 
>> I presume we need a WRITE_ONCE also upon initialization in
>> ovpn_peer_keepalive_set() right?
>> We still want to coordinate that with other reads/writes.
> 
> I think it makes sense, yes.

ACK

[...]
> 
>>>> +	/* 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?
> 
> We'll fail "delta < timeout" (which we shouldn't), so we'll end up
> either in the "expired = true" case, or not updating
> keepalive_recv_exp. Both of these seem not ideal.

delta is signed, so it'll end up being a negative value and "delta < 
timeout" should not fail then. Unless I am missing something.

Anyway, this was just an exercise to understand what was going on.
I already changed the code as per your suggestion (the fact that we are 
still discussing this chunk proves that it needed to be simplified :))

> 
>>
>> 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]
> 
> Ah, ok. I wanted to review this code when it was posted but didn't
> have time :(

It can still be fixed ;)


Thanks.
Regards,

-- 
Antonio Quartulli
OpenVPN Inc.