| Message ID | 20230111235052.24855-1-a@unstable.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] dco: print proper message in case of transport disconnection | expand |
Hi, > -/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ This specific hunk is beyond my pay grade. Otherwise looks good, I tested it with the latest dco master and got a proper error message: > SIGTERM[soft,ovpn-dco: transport disconnected] received, client-instance exiting Without this patch, the error message was > SIGTERM[soft,ovpn-dco: unknown reason] received, client-instance exiting Acked-by: Lev Stipakov <lstipakov@gmail.com>
On 12/01/2023 09:43, Lev Stipakov wrote: > Hi, > >> -/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > This specific hunk is beyond my pay grade. I dug into this. License wise, GPL-2.0 is the same as GPL-2.0-only in the spdx.dev [1] specification. But the change isn't correct according to SPDX specification standard v3; the short versions (GPL-*.*) was deprecated in favor of the longer version (GPL-*.*-only). I would let this change pass now, as it doesn't change the license itself. Later on this can be unified to a specific SPDX specification standard across all files. [1] <https://spdx.dev/licenses/> -- kind regards, David Sommerseth OpenVPN Inc
Am 12.01.23 um 09:43 schrieb Lev Stipakov: > Hi, > >> -/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > This specific hunk is beyond my pay grade. And we should change it. Currently that header file is incompatible with the rest of OpenVPN as it does not allow linking against OpenSSL. Arne
As discussed on IRC, there will be another patch with a license change
"really soon now". THIS patch is important to get into rc2, so people
can test new kernel modules and get proper DCO exit code reporting.
The patch itself is trivial ("compile tested so it won't accidentially
break something").
Your patch has been applied to the master and release/2.6 branch.
commit 67c4eebdaee5b51aa041ee7ed9f697397c04a01f (master)
commit 1a97b93a347bdc061a039048935d2f6827b3b5e8 (release/2.6)
Author: Antonio Quartulli
Date: Thu Jan 12 00:50:52 2023 +0100
dco: print proper message in case of transport disconnection
Signed-off-by: Antonio Quartulli <a@unstable.cc>
Acked-by: Lev Stipakov <lstipakov@gmail.com>
Message-Id: <20230111235052.24855-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25977.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, On Thu, Jan 12, 2023 at 12:50:52AM +0100, Antonio Quartulli wrote: > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index 77dcaa60..99123c39 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -3244,6 +3244,10 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, > reason = "ovpn-dco: transport error"; > break; > > + case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: > + reason = "ovpn-dco: transport disconnected"; > + break; > + We were too fast in ACKing and merging this "because rc2" - this breaks compilation on FreeBSD+DCO, because it only adds the necessary enum element for Linux. This being an enum, I can't even add an "#ifdef ...TRANSPORT_DISCONNECT" here, and because these codes need to be synchronized between kernel and userland, I can't "just add it to freebsd_dco.h". So consider me not very amused... As a quick bandaid, I'll send a patch that adds an #ifdef TARGET_LINUX around this clause so I can update the FreeBSD openvpn-devel port - but that is not a really good solution either. @kp, what would you suggest how to handle this on the FreeBSD side? gert
On 13 Jan 2023, at 20:35, Gert Doering wrote: > On Thu, Jan 12, 2023 at 12:50:52AM +0100, Antonio Quartulli wrote: >> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c >> index 77dcaa60..99123c39 100644 >> --- a/src/openvpn/multi.c >> +++ b/src/openvpn/multi.c >> @@ -3244,6 +3244,10 @@ process_incoming_del_peer(struct multi_context >> *m, struct multi_instance *mi, >> reason = "ovpn-dco: transport error"; >> break; >> >> + case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: >> + reason = "ovpn-dco: transport disconnected"; >> + break; >> + > > We were too fast in ACKing and merging this "because rc2" - this > breaks > compilation on FreeBSD+DCO, because it only adds the necessary enum > element for Linux. > > This being an enum, I can't even add an "#ifdef > ...TRANSPORT_DISCONNECT" > here, and because these codes need to be synchronized between kernel > and > userland, I can't "just add it to freebsd_dco.h". > > So consider me not very amused... > > As a quick bandaid, I'll send a patch that adds an #ifdef TARGET_LINUX > around this clause so I can update the FreeBSD openvpn-devel port - > but > that is not a really good solution either. @kp, what would you > suggest > how to handle this on the FreeBSD side? > I’m happy to add `OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT` to the FreeBSD `enum ovpn_del_reason`. The FreeBSD if_ovpn code won’t ever send it (because no TCP) but that’s not really a concern. It’ll build and work just fine. Going forward we should try to remember this, and coordinate additions like this. I’m not sure how we’d cope with supporting building on older releases though. Not a worry just yet, because FreeBSD main is the only version with DCO support in it, but it’ll be a problem sooner or later. For that I see two paths: either we change the enum (probably both ovpn_notif_type and ovpn_del_reason) to be defines, so we can `#ifdef OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT`, or we keep the enums and extend the autoconf code to check for us. That’s the sort of thing it’s supposed to be for, after all. Best regards, Kristof
Hi, On 13/01/2023 09:32, Kristof Provost wrote: > I’m not sure how we’d cope with supporting building on older releases > though. Not a worry just yet, because FreeBSD main is the only version > with DCO support in it, but it’ll be a problem sooner or later. OpenVPN ships its own "recent"| version of the if_ovpn.h file, no? So as long as the file contains all the enums it will compile just fine. Cheers,
Hi, On Fri, Jan 13, 2023 at 09:37:49AM +0100, Antonio Quartulli wrote: > On 13/01/2023 09:32, Kristof Provost wrote: > > I???m not sure how we???d cope with supporting building on older releases > > though. Not a worry just yet, because FreeBSD main is the only version > > with DCO support in it, but it???ll be a problem sooner or later. > > OpenVPN ships its own "recent"| version of the if_ovpn.h file, no? > So as long as the file contains all the enums it will compile just fine. Yes, these defines are mirrored in our dco_freebsd.h - so I could just add it there, but we need to be careful that we never have conflicting enums in OpenVPN dco_freebsd.h vs. kernel if_ovpn.h. Or, as Antonio explained to me how it works on linux uapi headers "enums are only ever appended to, never ever reordered, nothing is ever deleted". I'm open for anything that lets me ship rc2+patch as FreeBSD port :-) gert
Hi, On 13/01/2023 09:44, Gert Doering wrote: > Hi, > > On Fri, Jan 13, 2023 at 09:37:49AM +0100, Antonio Quartulli wrote: >> On 13/01/2023 09:32, Kristof Provost wrote: >>> I???m not sure how we???d cope with supporting building on older releases >>> though. Not a worry just yet, because FreeBSD main is the only version >>> with DCO support in it, but it???ll be a problem sooner or later. >> >> OpenVPN ships its own "recent"| version of the if_ovpn.h file, no? >> So as long as the file contains all the enums it will compile just fine. > > Yes, these defines are mirrored in our dco_freebsd.h - so I could just > add it there, but we need to be careful that we never have conflicting > enums in OpenVPN dco_freebsd.h vs. kernel if_ovpn.h. > > Or, as Antonio explained to me how it works on linux uapi headers "enums > are only ever appended to, never ever reordered, nothing is ever deleted". > > I'm open for anything that lets me ship rc2+patch as FreeBSD port :-) As I was saying on IRC, IMHO we took this path of using the same names for enums on linux and freebsd because it was convenient. But here we are hitting a shortcoming of this approach. If we assume that enums are not supposed to be the same across platforms (the latter may have different logic, or whatever), we could implement a conversion function that takes a platform specific enum as input and returns a userspace specific enum. Platform independent code will then always only deal with userspace enums and won't care about what is truly written in the platform-specific headers. This way we draw a clear boundary between "what kernel on platform X supports" and "what userspace openvpn can deal with". And situations like this should never happen again. This conversion function would be implemented in the platform specific code (i.e. dco_linux.c) Does it make sense?
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 77dcaa60..99123c39 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3244,6 +3244,10 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, reason = "ovpn-dco: transport error"; break; + case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: + reason = "ovpn-dco: transport disconnected"; + break; + case OVPN_DEL_PEER_REASON_USERSPACE: /* We assume that is ourselves. Unfortunately, sometimes these * events happen with enough delay that they can have an order of diff --git a/src/openvpn/ovpn_dco_linux.h b/src/openvpn/ovpn_dco_linux.h index beca1beb..f9a3b827 100644 --- a/src/openvpn/ovpn_dco_linux.h +++ b/src/openvpn/ovpn_dco_linux.h @@ -1,8 +1,8 @@ -/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ /* * OpenVPN data channel accelerator * - * Copyright (C) 2019-2021 OpenVPN, Inc. + * Copyright (C) 2019-2022 OpenVPN, Inc. * * Author: James Yonan <james@openvpn.net> * Antonio Quartulli <antonio@openvpn.net> @@ -85,6 +85,7 @@ enum ovpn_del_peer_reason { OVPN_DEL_PEER_REASON_USERSPACE, OVPN_DEL_PEER_REASON_EXPIRED, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR, + OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT, __OVPN_DEL_PEER_REASON_AFTER_LAST };
Signed-off-by: Antonio Quartulli <a@unstable.cc> --- --no-verify is required upon commit due to changes in ovpn_dco_linux.h Little logging improvement for https://github.com/OpenVPN/ovpn-dco/issues/9 --- src/openvpn/multi.c | 4 ++++ src/openvpn/ovpn_dco_linux.h | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-)