[Openvpn-devel] Handle (DCO) timeouts in client mode

Message ID 20220427074828.6283-2-kprovost@netgate.com
State Superseded
Headers show
Series
  • [Openvpn-devel] Handle (DCO) timeouts in client mode
Related show

Commit Message

Connor Edwards via Openvpn-devel April 27, 2022, 7:48 a.m.
From: Kristof Provost <kp@FreeBSD.org>

Handle the DCO driver telling us that the peer went away, even if we're
not running in multi-instance mode.

Signed-off-by:	Kristof Provost <kprovost@netgate.com>
---
 src/openvpn/forward.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Antonio Quartulli April 27, 2022, 7:55 a.m. | #1
Hi Kristof,

On 27/04/2022 09:48, Kristof Provost via Openvpn-devel wrote:
> From: Kristof Provost <kp@FreeBSD.org>
> 
> Handle the DCO driver telling us that the peer went away, even if we're
> not running in multi-instance mode.

thanks for catching and fixing this. It was indeed on the todo list, but 
well hidden under the rest :-D

> 
> Signed-off-by:	Kristof Provost <kprovost@netgate.com>
> ---
>   src/openvpn/forward.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 9ddea439..f60c56a8 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1111,6 +1111,12 @@ process_incoming_dco(struct context *c)
>   
>       dco_do_read(dco);
>   
> +    if (dco->dco_message_type == OVPN_NOTIF_DEL_PEER)

Not sure where this constant is coming from, but on ovpn_dco_linux.h we 
have OVPN_CMD_DEL_PEER (similar format as the following OVPN_CMD_PACKET).

Is that a format we can adhere to on BSD as well?


@Lev, just a heads up: does dco-win deal with this case? It would be 
good to have a look on your side too.

Regards,

> +    {
> +        trigger_ping_timeout_signal(c);
> +        return;
> +    }
> +
>       if (dco->dco_message_type != OVPN_CMD_PACKET)
>       {
>           msg(D_DCO_DEBUG, "%s: received message of type %u - ignoring", __func__,
Gert Doering April 27, 2022, 7:59 a.m. | #2
Hi,

On Wed, Apr 27, 2022 at 09:55:24AM +0200, Antonio Quartulli wrote:
> > --- a/src/openvpn/forward.c
> > +++ b/src/openvpn/forward.c
> > @@ -1111,6 +1111,12 @@ process_incoming_dco(struct context *c)
> >   
> >       dco_do_read(dco);
> >   
> > +    if (dco->dco_message_type == OVPN_NOTIF_DEL_PEER)
> 
> Not sure where this constant is coming from, but on ovpn_dco_linux.h we 
> have OVPN_CMD_DEL_PEER (similar format as the following OVPN_CMD_PACKET).

For "notifications coming from kernel", _NOTIF_ feels more logical
than _CMD_ ("commands going into kernel").  No?

gert
Antonio Quartulli April 27, 2022, 8:04 a.m. | #3
Hi,

On 27/04/2022 09:59, Gert Doering wrote:
> Hi,
> 
> On Wed, Apr 27, 2022 at 09:55:24AM +0200, Antonio Quartulli wrote:
>>> --- a/src/openvpn/forward.c
>>> +++ b/src/openvpn/forward.c
>>> @@ -1111,6 +1111,12 @@ process_incoming_dco(struct context *c)
>>>    
>>>        dco_do_read(dco);
>>>    
>>> +    if (dco->dco_message_type == OVPN_NOTIF_DEL_PEER)
>>
>> Not sure where this constant is coming from, but on ovpn_dco_linux.h we
>> have OVPN_CMD_DEL_PEER (similar format as the following OVPN_CMD_PACKET).
> 
> For "notifications coming from kernel", _NOTIF_ feels more logical
> than _CMD_ ("commands going into kernel").  No?

It's just the same message that can go both directions.

This is the same that the WiFi stack in the Linux kernel does with 
messages such as NEW_STA and DEL_STA.

So we're just re-using the same accepted pattern.

Regards,
Connor Edwards via Openvpn-devel April 27, 2022, 9:52 a.m. | #4
On 27 Apr 2022, at 9:55, Antonio Quartulli wrote:
> Hi Kristof,
>
> On 27/04/2022 09:48, Kristof Provost via Openvpn-devel wrote:
>> From: Kristof Provost <kp@FreeBSD.org>
>>
>> Handle the DCO driver telling us that the peer went away, even if we're
>> not running in multi-instance mode.
>
> thanks for catching and fixing this. It was indeed on the todo list, but well hidden under the rest :-D
>
I know how todo piles work all too well.

>>
>> Signed-off-by:	Kristof Provost <kprovost@netgate.com>
>> ---
>>   src/openvpn/forward.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
>> index 9ddea439..f60c56a8 100644
>> --- a/src/openvpn/forward.c
>> +++ b/src/openvpn/forward.c
>> @@ -1111,6 +1111,12 @@ process_incoming_dco(struct context *c)
>>        dco_do_read(dco);
>>  +    if (dco->dco_message_type == OVPN_NOTIF_DEL_PEER)
>
> Not sure where this constant is coming from, but on ovpn_dco_linux.h we have OVPN_CMD_DEL_PEER (similar format as the following OVPN_CMD_PACKET).
>
> Is that a format we can adhere to on BSD as well?
>
Good catch. OVPN_NOTIF_DEL_PEER  is specific to the FreeBSD DCO interface and shouldn’t be used in the generic openvpn code. It worked accidentally because it has the same value as OVPN_CMD_DEL_PEER. We should indeed be using OVPN_CMD_DEL_PEER here.

I’ll update the patch and probably submit both the freebsd dco patch and this together in a few hours.

Kristof
Lev Stipakov April 27, 2022, 12:32 p.m. | #5
Hi,

@Lev, just a heads up: does dco-win deal with this case? It would be
> good to have a look on your side too.
>

Good point - this functionality (dco keepalive timeout) works in openvpn3
but not in openvpn2.

dco-win notifies userspace about keepalive timeout by returning certain
error code (STATUS_CONNECTION_DISCONNECTED in kernel
which translates to GetLastError 64 ERROR_NETNAME_DELETED, don't ask me
why) in response to read request, so we should catch it
inside read_incoming_link() after link_socket_read() call. Will do.
Lev Stipakov April 27, 2022, 1:18 p.m. | #6
How about we'll add dco_check_timeout(int, context) to dco.h with platform
specific implementations?

Here is what I just did for Windows (Linux part is no-nop):
https://github.com/lstipakov/openvpn/commit/ce242896c621273578a446c5194d5ca6aee04237

ke 27. huhtik. 2022 klo 15.32 Lev Stipakov (lstipakov@gmail.com) kirjoitti:

> Hi,
>
> @Lev, just a heads up: does dco-win deal with this case? It would be
>> good to have a look on your side too.
>>
>
> Good point - this functionality (dco keepalive timeout) works in openvpn3
> but not in openvpn2.
>
> dco-win notifies userspace about keepalive timeout by returning certain
> error code (STATUS_CONNECTION_DISCONNECTED in kernel
> which translates to GetLastError 64 ERROR_NETNAME_DELETED, don't ask me
> why) in response to read request, so we should catch it
> inside read_incoming_link() after link_socket_read() call. Will do.
>
> --
> -Lev
>
Lev Stipakov April 27, 2022, 1:24 p.m. | #7
And of course I forgot non-dco dummy implementation (which we don't call
yet, because on Windows dco is always defined)

https://github.com/lstipakov/openvpn/commit/f223bef8449f15ff5de06acdfee16088b855c47a

ke 27. huhtik. 2022 klo 16.18 Lev Stipakov (lstipakov@gmail.com) kirjoitti:

> How about we'll add dco_check_timeout(int, context) to dco.h with platform
> specific implementations?
>
> Here is what I just did for Windows (Linux part is no-nop):
> https://github.com/lstipakov/openvpn/commit/ce242896c621273578a446c5194d5ca6aee04237
>
> ke 27. huhtik. 2022 klo 15.32 Lev Stipakov (lstipakov@gmail.com)
> kirjoitti:
>
>> Hi,
>>
>> @Lev, just a heads up: does dco-win deal with this case? It would be
>>> good to have a look on your side too.
>>>
>>
>> Good point - this functionality (dco keepalive timeout) works in openvpn3
>> but not in openvpn2.
>>
>> dco-win notifies userspace about keepalive timeout by returning certain
>> error code (STATUS_CONNECTION_DISCONNECTED in kernel
>> which translates to GetLastError 64 ERROR_NETNAME_DELETED, don't ask me
>> why) in response to read request, so we should catch it
>> inside read_incoming_link() after link_socket_read() call. Will do.
>>
>> --
>> -Lev
>>
>
>
> --
> -Lev
>

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 9ddea439..f60c56a8 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1111,6 +1111,12 @@  process_incoming_dco(struct context *c)
 
     dco_do_read(dco);
 
+    if (dco->dco_message_type == OVPN_NOTIF_DEL_PEER)
+    {
+        trigger_ping_timeout_signal(c);
+        return;
+    }
+
     if (dco->dco_message_type != OVPN_CMD_PACKET)
     {
         msg(D_DCO_DEBUG, "%s: received message of type %u - ignoring", __func__,