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
