[M] Change in openvpn[master]: route: Don't delete pre-existing host route to VPN server on disconnect

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1704?usp=email

to review the following change.


Change subject: route: Don't delete pre-existing host route to VPN server on disconnect
......................................................................

route: Don't delete pre-existing host route to VPN server on disconnect

When a host route to the VPN server already exists at connect time
(e.g. a manually configured static route), add_route3() returns true
for both "successfully added" and "already exists" but discards the
per-route RT_ADDED bit, since it operates on a local struct route_ipv4.
The caller in redirect_default_route_to_vpn() then sets RL_DID_LOCAL
unconditionally, and on disconnect undo_redirect_default_route_to_vpn()
asks del_route3() to delete it -- which forces RT_ADDED on a fresh struct,
bypassing delete_route()'s protective guard. The result: the pre-existing
route gets removed.

Change add_route3() to return RTA_SUCCESS / RTA_EEXIST / RTA_ERROR by
inspecting the local struct's RT_ADDED bit after add_route(). Only set
RL_DID_LOCAL on RTA_SUCCESS so a pre-existing route survives disconnect.
Apply the same fix to the DHCP/DNS bypass routes via a new
route_bypass.bypass_added bitmask so each entry is only deleted on undo
if we added it.

GitHub: #970

Change-Id: I21d782780b7f89b3061dd97500589a24415afc23
Signed-off-by: Lev Stipakov <[email protected]>
---
M src/openvpn/route.c
M src/openvpn/route.h
2 files changed, 39 insertions(+), 17 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/04/1704/1

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 81ce867..06e63e1 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -882,7 +882,10 @@
     return ret;
 }
 
-static bool
+/* Returns RTA_SUCCESS, RTA_EEXIST or RTA_ERROR. RTA_EEXIST means the
+ * route was already present in the system and was not added by us; the
+ * caller must not delete it on undo. */
+static int
 add_route3(in_addr_t network, in_addr_t netmask, in_addr_t gateway, const struct tuntap *tt,
            unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es,
            openvpn_net_ctx_t *ctx)
@@ -893,7 +896,11 @@
     r.network = network;
     r.netmask = netmask;
     r.gateway = gateway;
-    return add_route(&r, tt, flags, rgi, es, ctx);
+    if (!add_route(&r, tt, flags, rgi, es, ctx))
+    {
+        return RTA_ERROR;
+    }
+    return (r.flags & RT_ADDED) ? RTA_SUCCESS : RTA_EEXIST;
 }
 
 static void
@@ -915,14 +922,19 @@
                   unsigned int flags, const struct route_gateway_info *rgi,
                   const struct env_set *es, openvpn_net_ctx_t *ctx)
 {
-    int ret = true;
+    bool ret = true;
+    rb->bypass_added = 0;
     for (int i = 0; i < rb->n_bypass; ++i)
     {
         if (rb->bypass[i])
         {
-            ret = add_route3(rb->bypass[i], IPV4_NETMASK_HOST, gateway, tt, flags | ROUTE_REF_GW,
-                             rgi, es, ctx)
-                  && ret;
+            int status = add_route3(rb->bypass[i], IPV4_NETMASK_HOST, gateway, tt,
+                                    flags | ROUTE_REF_GW, rgi, es, ctx);
+            if (status == RTA_SUCCESS)
+            {
+                rb->bypass_added |= (1u << i);
+            }
+            ret = (status != RTA_ERROR) && ret;
         }
     }
     return ret;
@@ -933,15 +945,15 @@
                   unsigned int flags, const struct route_gateway_info *rgi,
                   const struct env_set *es, openvpn_net_ctx_t *ctx)
 {
-    int i;
-    for (i = 0; i < rb->n_bypass; ++i)
+    for (int i = 0; i < rb->n_bypass; ++i)
     {
-        if (rb->bypass[i])
+        if (rb->bypass[i] && (rb->bypass_added & (1u << i)))
         {
             del_route3(rb->bypass[i], IPV4_NETMASK_HOST, gateway, tt, flags | ROUTE_REF_GW, rgi, es,
                        ctx);
         }
     }
+    rb->bypass_added = 0;
 }
 
 static bool
@@ -996,12 +1008,16 @@
                 if ((rl->spec.flags & RTSA_REMOTE_HOST)
                     && rl->spec.remote_host != IPV4_INVALID_ADDR)
                 {
-                    ret = add_route3(rl->spec.remote_host, IPV4_NETMASK_HOST, rl->rgi.gateway.addr,
-                                     tt, flags | ROUTE_REF_GW, &rl->rgi, es, ctx);
-                    if (ret)
+                    int status = add_route3(rl->spec.remote_host, IPV4_NETMASK_HOST,
+                                            rl->rgi.gateway.addr, tt, flags | ROUTE_REF_GW,
+                                            &rl->rgi, es, ctx);
+                    /* only flag as ours-to-undo when we actually added it;
+                     * a pre-existing route (RTA_EEXIST) must survive disconnect */
+                    if (status == RTA_SUCCESS)
                     {
                         rl->iflags |= RL_DID_LOCAL;
                     }
+                    ret = (status != RTA_ERROR);
                 }
                 else
                 {
@@ -1020,13 +1036,15 @@
                 if (rl->flags & RG_DEF1)
                 {
                     /* add new default route (1st component) */
-                    ret = add_route3(0x00000000, 0x80000000, rl->spec.remote_endpoint, tt, flags,
-                                     &rl->rgi, es, ctx)
+                    ret = (add_route3(0x00000000, 0x80000000, rl->spec.remote_endpoint, tt, flags,
+                                      &rl->rgi, es, ctx)
+                           != RTA_ERROR)
                           && ret;
 
                     /* add new default route (2nd component) */
-                    ret = add_route3(0x80000000, 0x80000000, rl->spec.remote_endpoint, tt, flags,
-                                     &rl->rgi, es, ctx)
+                    ret = (add_route3(0x80000000, 0x80000000, rl->spec.remote_endpoint, tt, flags,
+                                      &rl->rgi, es, ctx)
+                           != RTA_ERROR)
                           && ret;
                 }
                 else
@@ -1040,7 +1058,8 @@
                     }
 
                     /* add new default route */
-                    ret = add_route3(0, 0, rl->spec.remote_endpoint, tt, flags, &rl->rgi, es, ctx)
+                    ret = (add_route3(0, 0, rl->spec.remote_endpoint, tt, flags, &rl->rgi, es, ctx)
+                           != RTA_ERROR)
                           && ret;
                 }
             }
diff --git a/src/openvpn/route.h b/src/openvpn/route.h
index 88107e9..23d2eed 100644
--- a/src/openvpn/route.h
+++ b/src/openvpn/route.h
@@ -54,6 +54,9 @@
 #define N_ROUTE_BYPASS 8
     int n_bypass;
     in_addr_t bypass[N_ROUTE_BYPASS];
+    /* bit i set when bypass[i] was actually added by us (so we may delete
+     * it on undo); cleared when the route already existed in the system. */
+    unsigned int bypass_added;
 };
 
 struct route_special_addr

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1704?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I21d782780b7f89b3061dd97500589a24415afc23
Gerrit-Change-Number: 1704
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel