| Message ID | 20220427074828.6283-2-kprovost@netgate.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Handle (DCO) timeouts in client mode | expand |
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__,
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
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,
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
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.
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 >
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 >
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__,
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(+)