[Openvpn-devel] [PATCH 2/2] Cleanup route error and debug logging on Windows
From: Selva Nair
Use a unified logging format for various route-methods
- Route add/delete errors are always logged with M_WARN, so
log only additional information (succeed/exists) with D_ROUTE.
- Non-windows platforms log route errors with a prefix "ERROR:" and
debug info with "ROUTE:". Do the same on Windows. Do not log
errors or success multiple times.
- In add_route_ipv6, log the interface id instead of device name
as the latter always point to the tun/tap adapter name on Windows.
Log lines prefixed with a PACKAGE_NAME "ROUTE" are unchanged.
They appear to use the same format on all platforms.
Signed-off-by: Selva Nair
---
src/openvpn/route.c | 63 +
1 file changed, 46 insertions(+), 17 deletions(-)
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index fe5cda2a..8117c648 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -1640,38 +1640,45 @@ add_route(struct route_ipv4 *r,
argv_msg(D_ROUTE, );
+const char *method = "service";
if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_SERVICE)
{
status = add_route_service(r, tt);
-msg(D_ROUTE, "Route addition via service %s", (status ==
RTA_SUCCESS) ? "succeeded" : "failed");
}
else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_IPAPI)
{
status = add_route_ipapi(r, tt, ai);
-msg(D_ROUTE, "Route addition via IPAPI %s", (status ==
RTA_SUCCESS) ? "succeeded" : "failed");
+method = "ipapi";
}
else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_EXE)
{
netcmd_semaphore_lock();
status = openvpn_execve_check(, es, 0, "ERROR: Windows route
add command failed");
netcmd_semaphore_release();
+method = "route.exe";
}
else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_ADAPTIVE)
{
status = add_route_ipapi(r, tt, ai);
-msg(D_ROUTE, "Route addition via IPAPI %s [adaptive]", (status ==
RTA_SUCCESS) ? "succeeded" : "failed");
+method = "ipapi [adaptive]";
if (status == RTA_ERROR)
{
msg(D_ROUTE, "Route addition fallback to route.exe");
netcmd_semaphore_lock();
status = openvpn_execve_check(, es, 0, "ERROR: Windows
route add command failed [adaptive]");
netcmd_semaphore_release();
+method = "route.exe";
}
}
else
{
ASSERT(0);
}
+if (status != RTA_ERROR) /* error is logged upstream */
+{
+msg(D_ROUTE, "Route addition via %s %s", method, (status ==
RTA_SUCCESS) ?
+"succeeded" : "failed because route exists");
+}
}
#elif defined (TARGET_SOLARIS)
@@ -1867,7 +1874,6 @@ add_route_ipv6(struct route_ipv6 *r6, const struct tuntap
*tt,
openvpn_net_ctx_t *ctx)
{
int status = 0;
-const char *device = tt->actual_name;
bool gateway_needed = false;
if (!(r6->flags & RT_DEFINED) )
@@ -1879,6 +1885,7 @@ add_route_ipv6(struct route_ipv6 *r6, const struct tuntap
*tt,
struct gc_arena gc = gc_new();
#ifndef _WIN32
+const char *device = tt->actual_name;
if (r6->iface != NULL) /* vpn server special route */
{
device = r6->iface;
@@ -1911,8 +1918,14 @@ add_route_ipv6(struct route_ipv6 *r6, const struct
tuntap *tt,
}
#endif
+#ifndef _WIN32
msg( M_INFO, "add_route_ipv6(%s/%d -> %s metric %d) dev %s",
network, r6->netbits, gateway, r6->metric, device );
+#else
+msg( M_INFO, "add_route_ipv6(%s/%d -> %s metric %d) IF %lu",
+ network, r6->netbits, gateway, r6->metric,
+ r6->adapter_index ? r6->adapter_index : tt->adapter_index);
+#endif
/*
* Filter out routes which are essentially no-ops
@@ -2851,14 +2864,16 @@ add_route_ipapi(const struct route_ipv4 *r, const
struct tuntap *tt, DWORD adapt
doublebreak:
if (status != NO_ERROR)
{
-msg(M_WARN, "ROUTE: route addition failed using
CreateIpForwardEntry: %s [status=%u if_index=%u]",
-strerror_win32(status, ),
-(unsigned int)status,
-(unsigned int)if_index);
if (status == ERROR_OBJECT_ALREADY_EXISTS)
{
ret = RTA_EEXIST;
}
+else
+{
+msg(M_WARN, "ERROR: route addition failed using
CreateIpForwardEntry: "
+"%s [status=%u if_index=%u]", strerror_win32(status,
),
+(unsigned int)status, (unsigned int)if_index);
+}
}
}
}
@@ -2894,7 +2909,7 @@ del_route_ipapi(const struct route_ipv4 *r, const struct
tuntap *tt)
}
else
[Openvpn-devel] [PATCH 1/2] Define and use macros for route addition status code
From: Selva Nair
- Instead of 0, 1, 2 use RTA_ERROR, RTA_SUCCESS, RTA_EEXIST
as the return code of route addition functions.
- Also fix a logging error: status -> (status == RTA_SUCCESS)
Signed-off-by: Selva Nair
---
src/openvpn/route.c | 106 ++--
1 file changed, 62 insertions(+), 44 deletions(-)
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 21dce6af..fe5cda2a 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -96,6 +96,11 @@ print_bypass_addresses(const struct route_bypass *rb)
#endif
+/* Route addition return status codes */
+#define RTA_ERROR 0 /* route addition failed */
+#define RTA_SUCCESS 1 /* route addition succeeded */
+#define RTA_EEXIST 2 /* route not added as it already exists */
+
static bool
add_bypass_address(struct route_bypass *rb, const in_addr_t a)
{
@@ -1593,12 +1598,12 @@ add_route(struct route_ipv4 *r,
metric = r->metric;
}
-status = 1;
+status = RTA_SUCCESS;
if (net_route_v4_add(ctx, >network, netmask_to_netbits2(r->netmask),
>gateway, iface, 0, metric) < 0)
{
msg(M_WARN, "ERROR: Linux route add command failed");
-status = 0;
+status = RTA_ERROR;
}
#elif defined (TARGET_ANDROID)
@@ -1612,7 +1617,7 @@ add_route(struct route_ipv4 *r,
{
openvpn_snprintf(out, sizeof(out), "%s %s %s", network, netmask,
gateway);
}
-status = management_android_control(management, "ROUTE", out);
+status = management_android_control(management, "ROUTE", out) ?
RTA_SUCCESS : RTA_ERROR;
#elif defined (_WIN32)
{
@@ -1638,12 +1643,12 @@ add_route(struct route_ipv4 *r,
if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_SERVICE)
{
status = add_route_service(r, tt);
-msg(D_ROUTE, "Route addition via service %s", (status == 1) ?
"succeeded" : "failed");
+msg(D_ROUTE, "Route addition via service %s", (status ==
RTA_SUCCESS) ? "succeeded" : "failed");
}
else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_IPAPI)
{
status = add_route_ipapi(r, tt, ai);
-msg(D_ROUTE, "Route addition via IPAPI %s", (status == 1) ?
"succeeded" : "failed");
+msg(D_ROUTE, "Route addition via IPAPI %s", (status ==
RTA_SUCCESS) ? "succeeded" : "failed");
}
else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_EXE)
{
@@ -1654,8 +1659,8 @@ add_route(struct route_ipv4 *r,
else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_ADAPTIVE)
{
status = add_route_ipapi(r, tt, ai);
-msg(D_ROUTE, "Route addition via IPAPI %s [adaptive]", (status ==
1) ? "succeeded" : "failed");
-if (status == 0)
+msg(D_ROUTE, "Route addition via IPAPI %s [adaptive]", (status ==
RTA_SUCCESS) ? "succeeded" : "failed");
+if (status == RTA_ERROR)
{
msg(D_ROUTE, "Route addition fallback to route.exe");
netcmd_semaphore_lock();
@@ -1694,7 +1699,8 @@ add_route(struct route_ipv4 *r,
}
argv_msg(D_ROUTE, );
-status = openvpn_execve_check(, es, 0, "ERROR: Solaris route add
command failed");
+bool ret = openvpn_execve_check(, es, 0, "ERROR: Solaris route add
command failed");
+status = ret ? RTA_SUCCESS : RTA_ERROR;
#elif defined(TARGET_FREEBSD)
@@ -1716,7 +1722,8 @@ add_route(struct route_ipv4 *r,
/* FIXME -- add on-link support for FreeBSD */
argv_msg(D_ROUTE, );
-status = openvpn_execve_check(, es, 0, "ERROR: FreeBSD route add
command failed");
+bool ret = openvpn_execve_check(, es, 0, "ERROR: FreeBSD route add
command failed");
+status = ret ? RTA_SUCCESS : RTA_ERROR;
#elif defined(TARGET_DRAGONFLY)
@@ -1738,7 +1745,8 @@ add_route(struct route_ipv4 *r,
/* FIXME -- add on-link support for Dragonfly */
argv_msg(D_ROUTE, );
-status = openvpn_execve_check(, es, 0, "ERROR: DragonFly route add
command failed");
+bool ret = openvpn_execve_check(, es, 0, "ERROR: DragonFly route add
command failed");
+status = ret ? RTA_SUCCESS : RTA_ERROR;
#elif defined(TARGET_DARWIN)
@@ -1770,7 +1778,8 @@ add_route(struct route_ipv4 *r,
}
argv_msg(D_ROUTE, );
-status = openvpn_execve_check(, es, 0, "ERROR: OS X route add command
failed");
+bool ret = openvpn_execve_check(, es, 0, "ERROR: OS X route add
command failed");
+status = ret ? RTA_SUCCESS : RTA_ERROR;
#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
@@ -1792,7 +1801,8 @@ add_route(struct route_ipv4 *r,
/* FIXME -- add on-link support for OpenBSD/NetBSD */
argv_msg(D_ROUTE, );
-status = openvpn_execve_check(, es, 0, "ERROR: OpenBSD/NetBSD route
add command failed");
+bool ret = openvpn_execve_check(, es, 0, "ERROR: OpenBSD/NetBSD route
add command failed");
+status = ret ? RTA_SUCCESS : RTA_ERROR;
Re: [Openvpn-devel] [PATCH] Repair special-casing of EEXIST for Linux/SITNL route install
On Wed, Jan 11, 2023 at 11:38 AM Gert Doering wrote: > The code in sitnl_route_set() used to treat "route can not be installed > because it already exists" (EEXIST) as "not an error". > > This is arguably a reasonable approach, but needs to handled higher > up - if the low level add_route() function say "no error", we will try > to remove that route later on in delete_route(), possibly removing > someone else's "already existing" route then. > Found one more case of route addition flag not correctly recorded -- RL_DID_LOCAL used for the direct route to the server. I will wait for the final shape and form of this one before sending a patch. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Fix OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT breakage on FreeBSD+DCO
Antonio, thanks for the ACK, and yes, we need a FreeBSD 14 buildbot (Github Actions has no support for "not yet released" OSes yet, as far as I'm aware) Patch has been applied to the master and release/2.6 branch. commit cf545d603ecd9fbacc6bd519efaa92d60f944287 (master) commit 480ad2a84e2983e8a1b61d537cf82da5c5141853 (release/2.6) Author: Gert Doering Date: Fri Jan 13 09:07:45 2023 +0100 Fix OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT breakage on FreeBSD+DCO Signed-off-by: Gert Doering Acked-by: Antonio Quartulli Message-Id: <20230113080745.82783-1-g...@greenie.muc.de> URL: https://www.mail-archive.com/search?l=mid=20230113080745.82783-1-g...@greenie.muc.de Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT breakage on FreeBSD+DCO
Hi, On 13/01/2023 09:07, Gert Doering wrote: commit 67c4eebdae introduces a new peer disconnect reason (transport disconnected, aka "TCP session closed") which breaks compilation on FreeBSD - OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT not part of the enum in freebsd_dco.h, and no kernel support for TCP anyway. This patch is an intermediate bandaid, making the offending code in multi.c "linux only" while a better solution is discussed. Signed-off-by: Gert Doering Hi, as we are discussing in a nother thread and on IRC, we need to come up with a better solution that is flexible enough to prevent this from happening in the future. There are some floating ideas. Anyway, the discussion will continue in the other thread. Regarding this patch: Acked-by: Antonio Quartulli Maybe we need a fbsd14 buildbot? Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: print proper message in case of transport disconnection
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? -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: print proper message in case of transport disconnection
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___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: print proper message in case of transport disconnection
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 -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: print proper message in case of transport disconnection
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, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Fix OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT breakage on FreeBSD+DCO
commit 67c4eebdae introduces a new peer disconnect reason (transport disconnected, aka "TCP session closed") which breaks compilation on FreeBSD - OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT not part of the enum in freebsd_dco.h, and no kernel support for TCP anyway. This patch is an intermediate bandaid, making the offending code in multi.c "linux only" while a better solution is discussed. Signed-off-by: Gert Doering --- src/openvpn/multi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 99123c39..f2559016 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3244,9 +3244,12 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, reason = "ovpn-dco: transport error"; break; +#ifdef TARGET_LINUX +/* FIXME: this is linux-only today and breaks FreeBSD compilation */ case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: reason = "ovpn-dco: transport disconnected"; break; +#endif case OVPN_DEL_PEER_REASON_USERSPACE: /* We assume that is ourselves. Unfortunately, sometimes these -- 2.38.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
