[Openvpn-devel,v2] Use lowest metric interface when multiple interfaces match a route

Message ID 1509930876-30728-1-git-send-email-selva.nair@gmail.com
State Superseded
Delegated to: Gert Doering
Headers show
Series
  • [Openvpn-devel,v2] Use lowest metric interface when multiple interfaces match a route
Related show

Commit Message

Selva Nair Nov. 6, 2017, 1:14 a.m.
From: Selva Nair <selva.nair@gmail.com>

Currently a route addition using IPAPI or service is skipped if the
route gateway is reachable by multiple interfaces. This changes that
to use the interface with lowest metric. Implemented by

(i)  Do not over-write the return value with TUN_ADAPTER_INDEX_INVALID in
     windows_route_find_if_index() if multiple interfaces match a route.
(ii) Select the interface with lowest metric in adapter_index_of_ip()
     instead of the first one found when multiple interfaces match.

Reported by Jan Just Keijser <janjust@nikhef.nl>

v2: - A private get_interface_metric() method and better error reporting
    - Revert an unintented edit of route.c (a_index = ...)
    - Improve the commit message

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/route.c |  1 -
 src/openvpn/tun.c   | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 57 insertions(+), 3 deletions(-)

Comments

Илья Шипицин Dec. 6, 2017, 1:28 p.m. | #1
2017-11-06 6:14 GMT+05:00 <selva.nair@gmail.com>:

> From: Selva Nair <selva.nair@gmail.com>
>
> Currently a route addition using IPAPI or service is skipped if the
> route gateway is reachable by multiple interfaces. This changes that
> to use the interface with lowest metric. Implemented by
>
> (i)  Do not over-write the return value with TUN_ADAPTER_INDEX_INVALID in
>      windows_route_find_if_index() if multiple interfaces match a route.
> (ii) Select the interface with lowest metric in adapter_index_of_ip()
>      instead of the first one found when multiple interfaces match.
>
> Reported by Jan Just Keijser <janjust@nikhef.nl>
>
> v2: - A private get_interface_metric() method and better error reporting
>     - Revert an unintented edit of route.c (a_index = ...)
>     - Improve the commit message
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
>  src/openvpn/route.c |  1 -
>  src/openvpn/tun.c   | 59 ++++++++++++++++++++++++++++++
> +++++++++++++++++++++--
>  2 files changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index 8c71e6e..66a8ae3 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -2780,7 +2780,6 @@ windows_route_find_if_index(const struct route_ipv4
> *r, const struct tuntap *tt)
>          msg(M_WARN, "Warning: route gateway is ambiguous: %s (%d
> matches)",
>              print_in_addr_t(r->gateway, 0, &gc),
>              count);
> -        ret = TUN_ADAPTER_INDEX_INVALID;
>      }
>
>      dmsg(D_ROUTE_DEBUG, "DEBUG: route find if: on_tun=%d count=%d
> index=%d",
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 3639718..7603133 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -4474,6 +4474,49 @@ is_ip_in_adapter_subnet(const IP_ADAPTER_INFO *ai,
> const in_addr_t ip, in_addr_t
>      return ret;
>  }
>
> +/**
> + * Given an interface index return the interface metric.
> + *
> + * Arguments:
> + *   index         : The index of the interface
> + *   family        : AF_INET for IPv4 or AF_INET6 for IPv6
> + * On error returns -1
> + */
> +
> +/* function signature missing in mingw iphlpapi.h */
>

that definition apparently present in mingw since 2015

https://sourceforge.net/p/mingw-w64/mingw-w64/ci/ea95d55e3387353e453d6ae8fc5cb8f7503947c2/tree/mingw-w64-headers/include/netioapi.h



> +VOID NETIOAPI_API_
> +InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row);
> +
> +static int
> +get_interface_metric(NET_IFINDEX index, ADDRESS_FAMILY family)
> +{
> +    DWORD err;
> +    int msglevel = D_ROUTE|M_WARN;
> +    MIB_IPINTERFACE_ROW ipiface;
> +
> +    InitializeIpInterfaceEntry(&ipiface);
> +    ipiface.Family = family;
> +    ipiface.InterfaceIndex = index;
> +
> +    err = GetIpInterfaceEntry(&ipiface);
> +    if (err == NO_ERROR)
> +    {
> +        return ipiface.Metric;
> +    }
> +    else if (err == ERROR_NOT_FOUND)
> +    {
> +        /*
> +         *  This happens if the address family is not enabled for the
> +         *  interface, which is benign -- display only at a debug level
> +         */
> +        msglevel = D_ROUTE_DEBUG;
> +    }
> +    msg(msglevel, "Note: failed to determine metric of interface "
> +                  "<%lu> for %s : (error code = %lu)",
> +                  index, (family == AF_INET)? "ipv4" : "ipv6", err);
> +    return -1;
> +}
> +
>  DWORD
>  adapter_index_of_ip(const IP_ADAPTER_INFO *list,
>                      const in_addr_t ip,
> @@ -4483,6 +4526,7 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
>      struct gc_arena gc = gc_new();
>      DWORD ret = TUN_ADAPTER_INDEX_INVALID;
>      in_addr_t highest_netmask = 0;
> +    int lowest_metric = INT_MAX;
>      bool first = true;
>
>      if (count)
> @@ -4496,9 +4540,14 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
>
>          if (is_ip_in_adapter_subnet(list, ip, &hn))
>          {
> +            int metric = get_interface_metric(list->Index, AF_INET);
>              if (first || hn > highest_netmask)
>              {
>                  highest_netmask = hn;
> +                if (metric >= 0)
> +                {
> +                    lowest_metric = metric;
> +                }
>                  if (count)
>                  {
>                      *count = 1;
> @@ -4512,16 +4561,22 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
>                  {
>                      ++*count;
>                  }
> +                if (metric >= 0 && metric < lowest_metric)
> +                {
> +                    ret = list->Index;
> +                    lowest_metric = metric;
> +                }
>              }
>          }
>          list = list->Next;
>      }
>
> -    dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d",
> +    dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d
> metric=%d",
>           print_in_addr_t(ip, 0, &gc),
>           print_in_addr_t(highest_netmask, 0, &gc),
>           (int)ret,
> -         count ? *count : -1);
> +         count ? *count : -1,
> +         lowest_metric);
>
>      if (ret == TUN_ADAPTER_INDEX_INVALID && count)
>      {
> --
> 2.1.4
>
>
> ------------------------------------------------------------
> ------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2017-11-06 6:14 GMT+05:00  <span dir="ltr">&lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;</span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt;<br>
<br>
Currently a route addition using IPAPI or service is skipped if the<br>
route gateway is reachable by multiple interfaces. This changes that<br>
</span>to use the interface with lowest metric. Implemented by<br>
<br>
(i)  Do not over-write the return value with TUN_ADAPTER_INDEX_INVALID in<br>
     windows_route_find_if_index() if multiple interfaces match a route.<br>
(ii) Select the interface with lowest metric in adapter_index_of_ip()<br>
     instead of the first one found when multiple interfaces match.<br>
<span class="gmail-"><br>
Reported by Jan Just Keijser &lt;<a href="mailto:janjust@nikhef.nl">janjust@nikhef.nl</a>&gt;<br>
<br>
</span>v2: - A private get_interface_metric() method and better error reporting<br>
    - Revert an unintented edit of route.c (a_index = ...)<br>
    - Improve the commit message<br>
<span class="gmail-"><br>
Signed-off-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt;<br>
---<br>
</span> src/openvpn/route.c |  1 -<br>
 src/openvpn/tun.c   | 59 ++++++++++++++++++++++++++++++<wbr>+++++++++++++++++++++--<br>
 2 files changed, 57 insertions(+), 3 deletions(-)<br>
<br>
diff --git a/src/openvpn/route.c b/src/openvpn/route.c<br>
index 8c71e6e..66a8ae3 100644<br>
--- a/src/openvpn/route.c<br>
+++ b/src/openvpn/route.c<br>
<span class="gmail-">@@ -2780,7 +2780,6 @@ windows_route_find_if_index(<wbr>const struct route_ipv4 *r, const struct tuntap *tt)<br>
         msg(M_WARN, &quot;Warning: route gateway is ambiguous: %s (%d matches)&quot;,<br>
             print_in_addr_t(r-&gt;gateway, 0, &amp;gc),<br>
             count);<br>
-        ret = TUN_ADAPTER_INDEX_INVALID;<br>
     }<br>
<br>
     dmsg(D_ROUTE_DEBUG, &quot;DEBUG: route find if: on_tun=%d count=%d index=%d&quot;,<br>
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c<br>
</span>index 3639718..7603133 100644<br>
--- a/src/openvpn/tun.c<br>
+++ b/src/openvpn/tun.c<br>
@@ -4474,6 +4474,49 @@ is_ip_in_adapter_subnet(const IP_ADAPTER_INFO *ai, const in_addr_t ip, in_addr_t<br>
     return ret;<br>
 }<br>
<br>
+/**<br>
+ * Given an interface index return the interface metric.<br>
+ *<br>
+ * Arguments:<br>
+ *   index         : The index of the interface<br>
+ *   family        : AF_INET for IPv4 or AF_INET6 for IPv6<br>
+ * On error returns -1<br>
+ */<br>
+<br>
+/* function signature missing in mingw iphlpapi.h */<br></blockquote><div><br></div><div>that definition apparently present in mingw since 2015<br><br><a href="https://sourceforge.net/p/mingw-w64/mingw-w64/ci/ea95d55e3387353e453d6ae8fc5cb8f7503947c2/tree/mingw-w64-headers/include/netioapi.h">https://sourceforge.net/p/mingw-w64/mingw-w64/ci/ea95d55e3387353e453d6ae8fc5cb8f7503947c2/tree/mingw-w64-headers/include/netioapi.h</a><br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+VOID NETIOAPI_API_<br>
+InitializeIpInterfaceEntry(<wbr>PMIB_IPINTERFACE_ROW Row);<br>
+<br>
+static int<br>
+get_interface_metric(NET_<wbr>IFINDEX index, ADDRESS_FAMILY family)<br>
+{<br>
+    DWORD err;<br>
+    int msglevel = D_ROUTE|M_WARN;<br>
+    MIB_IPINTERFACE_ROW ipiface;<br>
+<br>
+    InitializeIpInterfaceEntry(&amp;<wbr>ipiface);<br>
+    ipiface.Family = family;<br>
+    ipiface.InterfaceIndex = index;<br>
+<br>
+    err = GetIpInterfaceEntry(&amp;ipiface);<br>
+    if (err == NO_ERROR)<br>
+    {<br>
+        return ipiface.Metric;<br>
+    }<br>
+    else if (err == ERROR_NOT_FOUND)<br>
+    {<br>
+        /*<br>
+         *  This happens if the address family is not enabled for the<br>
+         *  interface, which is benign -- display only at a debug level<br>
+         */<br>
+        msglevel = D_ROUTE_DEBUG;<br>
+    }<br>
+    msg(msglevel, &quot;Note: failed to determine metric of interface &quot;<br>
+                  &quot;&lt;%lu&gt; for %s : (error code = %lu)&quot;,<br>
+                  index, (family == AF_INET)? &quot;ipv4&quot; : &quot;ipv6&quot;, err);<br>
+    return -1;<br>
+}<br>
+<br>
 DWORD<br>
 adapter_index_of_ip(const IP_ADAPTER_INFO *list,<br>
                     const in_addr_t ip,<br>
@@ -4483,6 +4526,7 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,<br>
<span class="gmail-">     struct gc_arena gc = gc_new();<br>
     DWORD ret = TUN_ADAPTER_INDEX_INVALID;<br>
     in_addr_t highest_netmask = 0;<br>
+    int lowest_metric = INT_MAX;<br>
     bool first = true;<br>
<br>
     if (count)<br>
</span>@@ -4496,9 +4540,14 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,<br>
<span class="gmail-"><br>
         if (is_ip_in_adapter_subnet(list, ip, &amp;hn))<br>
         {<br>
+            int metric = get_interface_metric(list-&gt;<wbr>Index, AF_INET);<br>
             if (first || hn &gt; highest_netmask)<br>
             {<br>
                 highest_netmask = hn;<br>
</span>+                if (metric &gt;= 0)<br>
+                {<br>
<span class="gmail-">+                    lowest_metric = metric;<br>
+                }<br>
</span><span class="gmail-">                 if (count)<br>
                 {<br>
                     *count = 1;<br>
</span>@@ -4512,16 +4561,22 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,<br>
                 {<br>
                     ++*count;<br>
                 }<br>
+                if (metric &gt;= 0 &amp;&amp; metric &lt; lowest_metric)<br>
<span class="gmail-">+                {<br>
+                    ret = list-&gt;Index;<br>
+                    lowest_metric = metric;<br>
+                }<br>
             }<br>
         }<br>
         list = list-&gt;Next;<br>
     }<br>
<br>
-    dmsg(D_ROUTE_DEBUG, &quot;DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d&quot;,<br>
+    dmsg(D_ROUTE_DEBUG, &quot;DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d metric=%d&quot;,<br>
          print_in_addr_t(ip, 0, &amp;gc),<br>
          print_in_addr_t(highest_<wbr>netmask, 0, &amp;gc),<br>
          (int)ret,<br>
-         count ? *count : -1);<br>
+         count ? *count : -1,<br>
</span>+         lowest_metric);<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br>
     if (ret == TUN_ADAPTER_INDEX_INVALID &amp;&amp; count)<br>
     {<br>
--<br>
2.1.4<br>
<br>
<br>
------------------------------<wbr>------------------------------<wbr>------------------<br>
Check out the vibrant tech community on one of the world&#39;s most<br>
engaging tech sites, Slashdot.org! <a href="http://sdm.link/slashdot" rel="noreferrer" target="_blank">http://sdm.link/slashdot</a><br>
______________________________<wbr>_________________<br>
Openvpn-devel mailing list<br>
<a href="mailto:Openvpn-devel@lists.sourceforge.net">Openvpn-devel@lists.<wbr>sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer" target="_blank">https://lists.sourceforge.net/<wbr>lists/listinfo/openvpn-devel</a><br>
</div></div></blockquote></div><br></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair Dec. 6, 2017, 3:01 p.m. | #2
On Wed, Dec 6, 2017 at 8:28 AM, Илья Шипицин <chipitsine@gmail.com> wrote:
>
>
> 2017-11-06 6:14 GMT+05:00 <selva.nair@gmail.com>:
>>
>>
..
>> +/**
>> + * Given an interface index return the interface metric.
>> + *
>> + * Arguments:
>> + *   index         : The index of the interface
>> + *   family        : AF_INET for IPv4 or AF_INET6 for IPv6
>> + * On error returns -1
>> + */
>> +
>> +/* function signature missing in mingw iphlpapi.h */
>
>
> that definition apparently present in mingw since 2015
>
> https://sourceforge.net/p/mingw-w64/mingw-w64/ci/ea95d55e3387353e453d6ae8fc5cb8f7503947c2/tree/mingw-w64-headers/include/netioapi.h
>
>
>>
>> +VOID NETIOAPI_API_
>> +InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row);
>>

2015 may sound old,  but we are targeting ubuntu 16.04 which has mingw 3.1.0
from mid 2014.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 8c71e6e..66a8ae3 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2780,7 +2780,6 @@  windows_route_find_if_index(const struct route_ipv4 *r, const struct tuntap *tt)
         msg(M_WARN, "Warning: route gateway is ambiguous: %s (%d matches)",
             print_in_addr_t(r->gateway, 0, &gc),
             count);
-        ret = TUN_ADAPTER_INDEX_INVALID;
     }
 
     dmsg(D_ROUTE_DEBUG, "DEBUG: route find if: on_tun=%d count=%d index=%d",
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 3639718..7603133 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -4474,6 +4474,49 @@  is_ip_in_adapter_subnet(const IP_ADAPTER_INFO *ai, const in_addr_t ip, in_addr_t
     return ret;
 }
 
+/**
+ * Given an interface index return the interface metric.
+ *
+ * Arguments:
+ *   index         : The index of the interface
+ *   family        : AF_INET for IPv4 or AF_INET6 for IPv6
+ * On error returns -1
+ */
+
+/* function signature missing in mingw iphlpapi.h */
+VOID NETIOAPI_API_
+InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row);
+
+static int
+get_interface_metric(NET_IFINDEX index, ADDRESS_FAMILY family)
+{
+    DWORD err;
+    int msglevel = D_ROUTE|M_WARN;
+    MIB_IPINTERFACE_ROW ipiface;
+
+    InitializeIpInterfaceEntry(&ipiface);
+    ipiface.Family = family;
+    ipiface.InterfaceIndex = index;
+
+    err = GetIpInterfaceEntry(&ipiface);
+    if (err == NO_ERROR)
+    {
+        return ipiface.Metric;
+    }
+    else if (err == ERROR_NOT_FOUND)
+    {
+        /*
+         *  This happens if the address family is not enabled for the
+         *  interface, which is benign -- display only at a debug level
+         */
+        msglevel = D_ROUTE_DEBUG;
+    }
+    msg(msglevel, "Note: failed to determine metric of interface "
+                  "<%lu> for %s : (error code = %lu)",
+                  index, (family == AF_INET)? "ipv4" : "ipv6", err);
+    return -1;
+}
+
 DWORD
 adapter_index_of_ip(const IP_ADAPTER_INFO *list,
                     const in_addr_t ip,
@@ -4483,6 +4526,7 @@  adapter_index_of_ip(const IP_ADAPTER_INFO *list,
     struct gc_arena gc = gc_new();
     DWORD ret = TUN_ADAPTER_INDEX_INVALID;
     in_addr_t highest_netmask = 0;
+    int lowest_metric = INT_MAX;
     bool first = true;
 
     if (count)
@@ -4496,9 +4540,14 @@  adapter_index_of_ip(const IP_ADAPTER_INFO *list,
 
         if (is_ip_in_adapter_subnet(list, ip, &hn))
         {
+            int metric = get_interface_metric(list->Index, AF_INET);
             if (first || hn > highest_netmask)
             {
                 highest_netmask = hn;
+                if (metric >= 0)
+                {
+                    lowest_metric = metric;
+                }
                 if (count)
                 {
                     *count = 1;
@@ -4512,16 +4561,22 @@  adapter_index_of_ip(const IP_ADAPTER_INFO *list,
                 {
                     ++*count;
                 }
+                if (metric >= 0 && metric < lowest_metric)
+                {
+                    ret = list->Index;
+                    lowest_metric = metric;
+                }
             }
         }
         list = list->Next;
     }
 
-    dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d",
+    dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d metric=%d",
          print_in_addr_t(ip, 0, &gc),
          print_in_addr_t(highest_netmask, 0, &gc),
          (int)ret,
-         count ? *count : -1);
+         count ? *count : -1,
+         lowest_metric);
 
     if (ret == TUN_ADAPTER_INDEX_INVALID && count)
     {