[Openvpn-devel] tun.c: do not add/remove routes on tun open/close

Message ID 20191219111821.313-1-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] tun.c: do not add/remove routes on tun open/close | expand

Commit Message

Lev Stipakov Dec. 19, 2019, 12:18 a.m. UTC
From: Lev Stipakov <lev@openvpn.net>

Commit 1c4a47f added route manipulation to open/close tun functions,
which is not really needed:

 - routes are added in check_add_routes(), triggered by timer

 - routes are removed in delete_routes(), called by do_close_tun()

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/route.c |  2 +-
 src/openvpn/route.h |  3 ---
 src/openvpn/tun.c   | 17 -----------------
 3 files changed, 1 insertion(+), 21 deletions(-)

Comments

Simon Rozman Dec. 19, 2019, 8:04 a.m. UTC | #1
Makes sense. I am pretty sure the interface network route is not required to
be added explicitly.

Acked-by: Simon Rozman <simon@rozman.si>

Best regards,
Simon

> -----Original Message-----
> From: Lev Stipakov <lstipakov@gmail.com>
> Sent: Thursday, December 19, 2019 12:18 PM
> To: openvpn-devel@lists.sourceforge.net
> Cc: Lev Stipakov <lev@openvpn.net>
> Subject: [Openvpn-devel] [PATCH] tun.c: do not add/remove routes on tun
> open/close
> 
> From: Lev Stipakov <lev@openvpn.net>
> 
> Commit 1c4a47f added route manipulation to open/close tun functions,
> which is not really needed:
> 
>  - routes are added in check_add_routes(), triggered by timer
> 
>  - routes are removed in delete_routes(), called by do_close_tun()
> 
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  src/openvpn/route.c |  2 +-
>  src/openvpn/route.h |  3 ---
>  src/openvpn/tun.c   | 17 -----------------
>  3 files changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c index
> cc6d5519..97e90e56 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -3019,7 +3019,7 @@ out:
>      return ret;
>  }
> 
> -bool
> +static bool
>  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); diff --git
> a/src/openvpn/route.h b/src/openvpn/route.h index 27b652cd..7dd96091
> 100644
> --- a/src/openvpn/route.h
> +++ b/src/openvpn/route.h
> @@ -321,9 +321,6 @@ void setenv_routes(struct env_set *es, const struct
> route_list *rl);
> 
>  void setenv_routes_ipv6(struct env_set *es, const struct
> route_ipv6_list *rl6);
> 
> -bool do_route_ipv4_service(const bool add, const struct route_ipv4 *r,
> -                           const struct tuntap *tt);
> -
>  bool is_special_addr(const char *addr_str);
> 
>  void get_default_gateway(struct route_gateway_info *rgi, diff --git
> a/src/openvpn/tun.c b/src/openvpn/tun.c index 8d87ac41..ad497a71 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -860,21 +860,6 @@ delete_route_connected_v6_net(struct tuntap *tt,  }
> #endif /* if defined(_WIN32) || defined(TARGET_DARWIN) ||
> defined(TARGET_NETBSD) || defined(TARGET_OPENBSD) */
> 
> -#if defined(_WIN32)
> -void
> -do_route_ipv4_service_tun(bool add, const struct tuntap *tt) -{
> -    struct route_ipv4 r4;
> -    CLEAR(r4);
> -    r4.network = tt->local & tt->remote_netmask;
> -    r4.netmask = tt->remote_netmask;
> -    r4.gateway = tt->local;
> -    r4.metric = 0;                     /* connected route */
> -    r4.flags = RT_DEFINED | RT_METRIC_DEFINED;
> -    do_route_ipv4_service(add, &r4, tt);
> -}
> -#endif
> -
>  #if defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)  \
>      || defined(TARGET_NETBSD) || defined(TARGET_OPENBSD)
>  /* we can't use true subnet mode on tun on all platforms, as that @@ -
> 1406,7 +1391,6 @@ do_ifconfig_ipv4(struct tuntap *tt, const char
> *ifname, int tun_mtu,
>          if (tt->options.msg_channel && tt->wintun)
>          {
>              do_address_service(true, AF_INET, tt);
> -            do_route_ipv4_service_tun(true, tt);
>              do_dns_service(true, AF_INET, tt);
>          }
>          else
> @@ -6489,7 +6473,6 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t
> *ctx)
>      {
>          if (tt->options.msg_channel)
>          {
> -            do_route_ipv4_service_tun(false, tt);
>              do_address_service(false, AF_INET, tt);
>              do_dns_service(false, AF_INET, tt);
>          }
> --
> 2.17.1
> 
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering Dec. 19, 2019, 9:10 a.m. UTC | #2
Your patch has been applied to the master branch.

As discussed on the IRC meeting, I've amended the commit message to make
it more clear what happened.  See below.

(Test built on ubuntu 1604, and also stared-at-code and asked confused
questions :-) )

commit 4f2289893d9d1d448e9654b00d18e13f8fb0936c
Author: Lev Stipakov
Date:   Thu Dec 19 13:18:21 2019 +0200

     tun.c: do not add/remove on-link IPv4 route on tun open/close
    
     Commit 1c4a47f added route manipulation to open/close tun functions
     (for the IPv4 on-link network), but it turns out to be not needed
     at all.

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Simon Rozman <simon@rozman.si>
     Message-Id: <20191219111821.313-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19256.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index cc6d5519..97e90e56 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -3019,7 +3019,7 @@  out:
     return ret;
 }
 
-bool
+static bool
 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);
diff --git a/src/openvpn/route.h b/src/openvpn/route.h
index 27b652cd..7dd96091 100644
--- a/src/openvpn/route.h
+++ b/src/openvpn/route.h
@@ -321,9 +321,6 @@  void setenv_routes(struct env_set *es, const struct route_list *rl);
 
 void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6);
 
-bool do_route_ipv4_service(const bool add, const struct route_ipv4 *r,
-                           const struct tuntap *tt);
-
 bool is_special_addr(const char *addr_str);
 
 void get_default_gateway(struct route_gateway_info *rgi,
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 8d87ac41..ad497a71 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -860,21 +860,6 @@  delete_route_connected_v6_net(struct tuntap *tt,
 }
 #endif /* if defined(_WIN32) || defined(TARGET_DARWIN) || defined(TARGET_NETBSD) || defined(TARGET_OPENBSD) */
 
-#if defined(_WIN32)
-void
-do_route_ipv4_service_tun(bool add, const struct tuntap *tt)
-{
-    struct route_ipv4 r4;
-    CLEAR(r4);
-    r4.network = tt->local & tt->remote_netmask;
-    r4.netmask = tt->remote_netmask;
-    r4.gateway = tt->local;
-    r4.metric = 0;                     /* connected route */
-    r4.flags = RT_DEFINED | RT_METRIC_DEFINED;
-    do_route_ipv4_service(add, &r4, tt);
-}
-#endif
-
 #if defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)  \
     || defined(TARGET_NETBSD) || defined(TARGET_OPENBSD)
 /* we can't use true subnet mode on tun on all platforms, as that
@@ -1406,7 +1391,6 @@  do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,
         if (tt->options.msg_channel && tt->wintun)
         {
             do_address_service(true, AF_INET, tt);
-            do_route_ipv4_service_tun(true, tt);
             do_dns_service(true, AF_INET, tt);
         }
         else
@@ -6489,7 +6473,6 @@  close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
     {
         if (tt->options.msg_channel)
         {
-            do_route_ipv4_service_tun(false, tt);
             do_address_service(false, AF_INET, tt);
             do_dns_service(false, AF_INET, tt);
         }