diff mbox series

[Openvpn-devel,v2,2/3] Distinguish route addition errors from route already exists

Message ID 20230106150412.1667492-1-selva.nair@gmail.com
State Accepted
Headers show
Series None | expand

Commit Message

Selva Nair Jan. 6, 2023, 3:04 p.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

When possible, functions that add a route now return 1 on success,
or 2 if route already exists or 0 on other errors instead of true/false.

Note:
net_route_v4/v6_add using netlink filters out EEXIST before returning
this looks like a bug as add_route() and add_route_ipv6() should set
RT_ADDED only if route was really added.

v2: "succeeded/skipped" --> "succeeded" in log.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
net_route_v4/v6_add when using iproute2 always returns 0 (meaning success)
making it impossible to respect RT_ADDED flag or propagate errors. I think
these should be fixed.

 src/openvpn/route.c | 91 ++++++++++++++++++++++++++++-----------------
 src/openvpn/route.h |  4 --
 2 files changed, 57 insertions(+), 38 deletions(-)

Comments

Lev Stipakov Jan. 9, 2023, 7:50 a.m. UTC | #1 
I reviewed v1, checked that true/false has been replaced with 0/1/2.
Checked that the only change in v2 is that "succeeded/skipped" is
replaced with "succeeded".

Compiled and tested, looks good.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gert Doering Jan. 9, 2023, 11:52 a.m. UTC | #2 
Hi,

On Fri, Jan 06, 2023 at 10:04:12AM -0500, selva.nair@gmail.com wrote:
> Note:
> net_route_v4/v6_add using netlink filters out EEXIST before returning
> this looks like a bug as add_route() and add_route_ipv6() should set
> RT_ADDED only if route was really added.

Actually we do have a Trac ticket that might be related to this
somewhere, about multiple instances of OpenVPN messing each other's
routing that way...  maybe it's that very issue, maybe not.  But worth
having a very close look.

gert
Gert Doering Jan. 9, 2023, 12:17 p.m. UTC | #3 
Acked-by: Gert Doering <gert@greenie.muc.de>

Lev has also ACKed, but since I did some previous digging into the code,
I gave it an extra-hard stare and can confirm that it looks very reasonable.

That said, I have a dislike for magic "0", "1" and "2" constants appearing
with no explanation - can we have a followup patch that documents the
expected behaviour (maybe in route.h?) and also introduces some helpful
constants?  Like, we have ISC_ERROR, maybe RTA_ERROR, RTA_SUCCESS, 
RTA_EEXISTS or so?  I do not care much about the actual naming, but 
I'm sure that I'll wonder in a year "why '2'?  what is '2' here?".

I also notice that there is quite a bit of imbalance between IPv4 and
IPv6 route addition on Windows - for IPv6, add_route_ipv6() will not
print any messages (so, no diff here), while IPv4 has all that...
maybe something to clean up in 2.7.

We *also* might want to get rid of that obscure 

  /* failed, try increasing the metric to work around Vista issue */

code bit, which looks extremely scary ("increasing metric with +1 in
a tight loop until we reach 2048") and Vista is out of support anyway...


Last not least, a 'status ? "succeeded"...' msg() escaped your
attention, in do_route_ipv6_service()...


I have compile-tested this for Windows, and client-side tested on 
Linux (netlink).  Unsurprisingly nothing really interesting happened,
as there is no real new code, my test rig has no "EEXIST" yet, and
nothing uses the new status values yet.


Your patch has been applied to the master and release/2.6 branch.

commit 9c6d72c783f4212f965e1e855b4fdb0ea34b595b (master)
commit 93da80eaa1548197b173537bb74bf6db7e10912e (release/2.6)
Author: Selva Nair
Date:   Fri Jan 6 10:04:12 2023 -0500

     Distinguish route addition errors from route already exists

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20230106150412.1667492-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25903.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Selva Nair Jan. 10, 2023, 12:56 a.m. UTC | #4 
Hi

On Mon, Jan 9, 2023 at 7:17 AM Gert Doering <gert@greenie.muc.de> wrote:

>
> I also notice that there is quite a bit of imbalance between IPv4 and
> IPv6 route addition on Windows - for IPv6, add_route_ipv6() will not
> print any messages (so, no diff here), while IPv4 has all that...
> maybe something to clean up in 2.7.
>
> We *also* might want to get rid of that obscure
>
>   /* failed, try increasing the metric to work around Vista issue */
>
> code bit, which looks extremely scary ("increasing metric with +1 in
> a tight loop until we reach 2048") and Vista is out of support anyway...
>

In fact the issue worked around is not a Vista thing but Vista+ including
Win10 and results from wrong use of the API. The old API used here requires
the metric parameter to be the route metric  + interface metric starting
Vista+.  So metric = 0 or any value < interface-metric fails. If we want a
metric of 5 on an interface that has a metric of 20, we have to pass in 25.

Looks like someone (I won't say who) added that obnoxious loop trying
metric from 0 to 2048 to work around this. Probably the idea was to support
pre-vista and vista without needing a run-time check.

That said, in Vista+ one should use CreateIpForwardEntry2() which is much
easier. As we do for IPv6 and for both v4 and v6 in the service.

Let's rewrite this function in 2.7.

Selva
diff mbox series

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index eabfe0a5..0d4aeaaf 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -57,15 +57,20 @@ 
 #include "openvpn-msg.h"
 
 #define METRIC_NOT_USED ((DWORD)-1)
-static bool add_route_service(const struct route_ipv4 *, const struct tuntap *);
+static int add_route_service(const struct route_ipv4 *, const struct tuntap *);
 
 static bool del_route_service(const struct route_ipv4 *, const struct tuntap *);
 
-static bool add_route_ipv6_service(const struct route_ipv6 *, const struct tuntap *);
+static int add_route_ipv6_service(const struct route_ipv6 *, const struct tuntap *);
 
 static bool del_route_ipv6_service(const struct route_ipv6 *, const struct tuntap *);
 
-static bool route_ipv6_ipapi(bool add, const struct route_ipv6 *, const struct tuntap *);
+static int route_ipv6_ipapi(bool add, const struct route_ipv6 *, const struct tuntap *);
+
+static int add_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt, DWORD adapter_index);
+
+static bool del_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt);
+
 
 #endif
 
@@ -1572,7 +1577,7 @@  add_route(struct route_ipv4 *r,
           const struct env_set *es,
           openvpn_net_ctx_t *ctx)
 {
-    bool status = false;
+    int status = 0;
     int is_local_route;
 
     if (!(r->flags & RT_DEFINED))
@@ -1611,12 +1616,12 @@  add_route(struct route_ipv4 *r,
         metric = r->metric;
     }
 
-    status = true;
+    status = 1;
     if (net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask),
                          &r->gateway, iface, 0, metric) < 0)
     {
         msg(M_WARN, "ERROR: Linux route add command failed");
-        status = false;
+        status = 0;
     }
 
 #elif defined (TARGET_ANDROID)
@@ -1656,12 +1661,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 ? "succeeded" : "failed");
+            msg(D_ROUTE, "Route addition via service %s", (status == 1) ? "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 ? "succeeded" : "failed");
+            msg(D_ROUTE, "Route addition via IPAPI %s", (status == 1) ? "succeeded" : "failed");
         }
         else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_EXE)
         {
@@ -1672,8 +1677,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 ? "succeeded" : "failed");
-            if (!status)
+            msg(D_ROUTE, "Route addition via IPAPI %s [adaptive]", (status == 1) ? "succeeded" : "failed");
+            if (status == 0)
             {
                 msg(D_ROUTE, "Route addition fallback to route.exe");
                 netcmd_semaphore_lock();
@@ -1828,7 +1833,7 @@  add_route(struct route_ipv4 *r,
 #endif /* if defined(TARGET_LINUX) */
 
 done:
-    if (status)
+    if (status == 1)
     {
         r->flags |= RT_ADDED;
     }
@@ -1871,7 +1876,7 @@  add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt,
                unsigned int flags, const struct env_set *es,
                openvpn_net_ctx_t *ctx)
 {
-    bool status = false;
+    int status = 0;
     const char *device = tt->actual_name;
     bool gateway_needed = false;
 
@@ -1942,7 +1947,7 @@  add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt,
             "parameter for a --route-ipv6 option and no default was set via "
             "--ifconfig-ipv6 or --route-ipv6-gateway option.  Not installing "
             "IPv6 route to %s/%d.", network, r6->netbits);
-        status = false;
+        status = 0;
         goto done;
     }
 
@@ -1953,13 +1958,13 @@  add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt,
         metric = r6->metric;
     }
 
-    status = true;
+    status = 1;
     if (net_route_v6_add(ctx, &r6->network, r6->netbits,
                          gateway_needed ? &r6->gateway : NULL, device, 0,
                          metric) < 0)
     {
         msg(M_WARN, "ERROR: Linux IPv6 route can't be added");
-        status = false;
+        status = 0;
     }
 
 #elif defined (TARGET_ANDROID)
@@ -2075,7 +2080,7 @@  add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt,
 #endif /* if defined(TARGET_LINUX) */
 
 done:
-    if (status)
+    if (status == 1)
     {
         r6->flags |= RT_ADDED;
     }
@@ -2773,11 +2778,11 @@  done:
     gc_free(&gc);
 }
 
-bool
+static int
 add_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt, DWORD adapter_index)
 {
     struct gc_arena gc = gc_new();
-    bool ret = false;
+    int ret = 0;
     DWORD status;
     const DWORD if_index = (adapter_index == TUN_ADAPTER_INDEX_INVALID) ? windows_route_find_if_index(r, tt) : adapter_index;
 
@@ -2811,7 +2816,11 @@  add_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt, DWORD adapt
 
         if (status == NO_ERROR)
         {
-            ret = true;
+            ret = 1;
+        }
+        else if (status == ERROR_OBJECT_ALREADY_EXISTS)
+        {
+            ret = 2;
         }
         else
         {
@@ -2830,7 +2839,7 @@  add_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt, DWORD adapt
                         msg(D_ROUTE, "ROUTE: CreateIpForwardEntry succeeded with dwForwardMetric1=%u and dwForwardType=%u",
                             (unsigned int)fr.dwForwardMetric1,
                             (unsigned int)fr.dwForwardType);
-                        ret = true;
+                        ret = 1;
                         goto doublebreak;
                     }
                     else if (status != ERROR_BAD_ARGUMENTS)
@@ -2847,6 +2856,10 @@  doublebreak:
                     strerror_win32(status, &gc),
                     (unsigned int)status,
                     (unsigned int)if_index);
+                if (status == ERROR_OBJECT_ALREADY_EXISTS)
+                {
+                    ret = 2;
+                }
             }
         }
     }
@@ -2855,7 +2868,7 @@  doublebreak:
     return ret;
 }
 
-bool
+static bool
 del_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt)
 {
     struct gc_arena gc = gc_new();
@@ -2891,10 +2904,11 @@  del_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt)
     return ret;
 }
 
-static bool
+/* Returns 1 on success, 2 if route exists, 0 on error */
+static int
 do_route_service(const bool add, const route_message_t *rt, const size_t size, HANDLE pipe)
 {
-    bool ret = false;
+    int ret = 0;
     ack_message_t ack;
     struct gc_arena gc = gc_new();
 
@@ -2908,23 +2922,25 @@  do_route_service(const bool add, const route_message_t *rt, const size_t size, H
         msg(M_WARN, "ROUTE: route %s failed using service: %s [status=%u if_index=%d]",
             (add ? "addition" : "deletion"), strerror_win32(ack.error_number, &gc),
             ack.error_number, rt->iface.index);
+        ret = (ack.error_number == ERROR_OBJECT_ALREADY_EXISTS) ? 2 : 0;
         goto out;
     }
 
-    ret = true;
+    ret = 1;
 
 out:
     gc_free(&gc);
     return ret;
 }
 
-static bool
+/* Returns 1 on success, 2 if route exists, 0 on error */
+static int
 do_route_ipv4_service(const bool add, const struct route_ipv4 *r, const struct tuntap *tt)
 {
     DWORD if_index = windows_route_find_if_index(r, tt);
     if (if_index == ~0)
     {
-        return false;
+        return 0;
     }
 
     route_message_t msg = {
@@ -2949,11 +2965,14 @@  do_route_ipv4_service(const bool add, const struct route_ipv4 *r, const struct t
     return do_route_service(add, &msg, sizeof(msg), tt->options.msg_channel);
 }
 
-/* Add or delete an ipv6 route */
-static bool
+/* Add or delete an ipv6 route
+ * Returns 1 on success, 2 if route exists, 0 on error
+ */
+static int
 route_ipv6_ipapi(const bool add, const struct route_ipv6 *r, const struct tuntap *tt)
 {
     DWORD err;
+    int ret = 0;
     PMIB_IPFORWARD_ROW2 fwd_row;
     struct gc_arena gc = gc_new();
 
@@ -3006,20 +3025,22 @@  out:
     {
         msg(M_WARN, "ROUTE: route %s failed using ipapi: %s [status=%lu if_index=%lu]",
             (add ? "addition" : "deletion"), strerror_win32(err, &gc), err, fwd_row->InterfaceIndex);
+        ret = (err == ERROR_OBJECT_ALREADY_EXISTS) ? 2 : 0;
     }
     else
     {
         msg(D_ROUTE, "IPv6 route %s using ipapi", add ? "added" : "deleted");
+        ret = 1;
     }
     gc_free(&gc);
-
-    return (err == NO_ERROR);
+    return ret;
 }
 
-static bool
+/* Returns 1 on success, 2 if route exists, 0 on error */
+static int
 do_route_ipv6_service(const bool add, const struct route_ipv6 *r, const struct tuntap *tt)
 {
-    bool status;
+    int status;
     route_message_t msg = {
         .header = {
             (add ? msg_add_route : msg_del_route),
@@ -3062,7 +3083,8 @@  do_route_ipv6_service(const bool add, const struct route_ipv6 *r, const struct t
     return status;
 }
 
-static bool
+/* Returns 1 on success, 2 if route exists, 0 on error */
+static int
 add_route_service(const struct route_ipv4 *r, const struct tuntap *tt)
 {
     return do_route_ipv4_service(true, r, tt);
@@ -3074,7 +3096,8 @@  del_route_service(const struct route_ipv4 *r, const struct tuntap *tt)
     return do_route_ipv4_service(false, r, tt);
 }
 
-static bool
+/* Returns 1 on success, 2 if route exists, 0 on error */
+static int
 add_route_ipv6_service(const struct route_ipv6 *r, const struct tuntap *tt)
 {
     return do_route_ipv6_service(true, r, tt);
diff --git a/src/openvpn/route.h b/src/openvpn/route.h
index 33f2b28e..74ecd343 100644
--- a/src/openvpn/route.h
+++ b/src/openvpn/route.h
@@ -357,10 +357,6 @@  void show_routes(int msglev);
 
 bool test_routes(const struct route_list *rl, const struct tuntap *tt);
 
-bool add_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt, DWORD adapter_index);
-
-bool del_route_ipapi(const struct route_ipv4 *r, const struct tuntap *tt);
-
 #else  /* ifdef _WIN32 */
 static inline bool
 test_routes(const struct route_list *rl, const struct tuntap *tt)