Skip to contentSkip to conversation

Set a low interface metric for tap adapter when block-outside-dns is in use

PATCH Superseded ValdikSS 2 participants 7 messages May 4, 2017 – May 10, 2017

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

ValdikSS

author
#0 lore raw

PATCH 1/1 Set a low interface metric for tap adapter when block-outside-dns is in use

From: ValdikSS <[email protected]...>

Windows 10 before Creators Update used to resolve DNS using all available adapters and IP addresses in parallel. Now it still resolves addresses using all available adapters but in a round-robin way, beginning with random adapter. This behaviour introduces significant delay when block-outside-dns is in use.
Fortunately, setting low metric for the TAP interface solves this issue, making Windows always pick with TAP adapter first.

---
 src/openvpn/block_dns.c       | 75 +++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/block_dns.h       | 30 +++++++++++++++++
 src/openvpn/init.c            |  4 +--
 src/openvpn/win32.c           | 29 +++++++++++++++--
 src/openvpn/win32.h           |  2 +-
 src/openvpnserv/interactive.c | 61 ++++++++++++++++++++++++++++++++---
 6 files changed, 191 insertions(+), 10 deletions(-)
src/openvpn/block_dns.c +75 -0
index e31765e..9d49355 100644
@@ -341,4 +341,79 @@ delete_block_dns_filters(HANDLE engine_handle)
     return err;
 }
 
+/* 
+ * Returns interface metric value for specified interface index.
+ * 
+ * Arguments:
+ *   index         : The index of TAP adapter.
+ *   family        : Address family (AF_INET for IPv4 and AF_INET6 for IPv6).
+ * Returns positive metric value or zero for automatic metric on success,
+ * a less then zero error code on failure.
+ */
+
+int
+get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family)
+{
+    DWORD err = 0;
+    MIB_IPINTERFACE_ROW ipiface;
+    InitializeIpInterfaceEntry(&ipiface);
+    ipiface.Family = family;
+    ipiface.InterfaceIndex = index;
+    err = GetIpInterfaceEntry(&ipiface);
+    if (err == NO_ERROR)
+    {
+        if (ipiface.UseAutomaticMetric)
+        {
+            return 0;
+        }
+        return ipiface.Metric;
+    }
+    return -err;
+}
+
+/* 
+ * Sets interface metric value for specified interface index.
+ * 
+ * Arguments:
+ *   index         : The index of TAP adapter.
+ *   family        : Address family (AF_INET for IPv4 and AF_INET6 for IPv6).
+ *   metric        : Metric value. 0 for automatic metric.
+ * Returns 0 on success, a non-zero status code of the last failed action on failure.
+ */
+
+DWORD
+set_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family,
+                     const ULONG metric)
+{
+    DWORD err = 0;
+    MIB_IPINTERFACE_ROW ipiface;
+    InitializeIpInterfaceEntry(&ipiface);
+    ipiface.Family = family;
+    ipiface.InterfaceIndex = index;
+    err = GetIpInterfaceEntry(&ipiface);
+    if (err == NO_ERROR)
+    {
+        if (family == AF_INET)
+        {
+            /* required for IPv4 as per MSDN */
+            ipiface.SitePrefixLength = 0;
+        }
+        ipiface.Metric = metric;
+        if (metric == 0)
+        {
+            ipiface.UseAutomaticMetric = TRUE;
+        }
+        else
+        {
+            ipiface.UseAutomaticMetric = FALSE;
+        }
+        err = SetIpInterfaceEntry(&ipiface);
+        if (err == NO_ERROR)
+        {
+            return 0;
+        }
+    }
+    return err;
+}
+
 #endif /* ifdef _WIN32 */
src/openvpn/block_dns.h +30 -0
index a7dadc4..e7b1cca 100644
@@ -27,6 +27,9 @@
 #ifndef OPENVPN_BLOCK_DNS_H
 #define OPENVPN_BLOCK_DNS_H
 
+/* Any value less than 5 should work fine. 3 is choosen without any real reason. */
+#define BLOCK_DNS_IFACE_METRIC 3
+
 typedef void (*block_dns_msg_handler_t) (DWORD err, const char *msg);
 
 DWORD
@@ -36,5 +39,32 @@ DWORD
 add_block_dns_filters(HANDLE *engine, int iface_index, const WCHAR *exe_path,
                       block_dns_msg_handler_t msg_handler_callback);
 
+/**
+ * Sets interface metric value for specified interface index
+ *
+ * @param index The index of TAP adapter
+ * @param family Address family (AF_INET for IPv4 and AF_INET6 for IPv6)
+ * 
+ * @return positive metric value or zero for automatic metric on success,
+ * a less then zero error code on failure.
+ */
+
+int
+get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family);
+
+/**
+ * Sets interface metric value for specified interface index
+ *
+ * @param index The index of TAP adapter
+ * @param family Address family (AF_INET for IPv4 and AF_INET6 for IPv6)
+ * @param metric Metric value. 0 for automatic metric
+ * 
+ * @return 0 on success, a non-zero status code of the last failed action on failure.
+ */
+
+DWORD
+set_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family,
+                     const ULONG metric);
+
 #endif
 #endif
src/openvpn/init.c +2 -2
index ee14f67..2a2e7b0 100644
@@ -1837,7 +1837,7 @@ do_close_tun(struct context *c, bool force)
 #if defined(_WIN32)
             if (c->options.block_outside_dns)
             {
-                if (!win_wfp_uninit(c->options.msg_channel))
+                if (!win_wfp_uninit(adapter_index, c->options.msg_channel))
                 {
                     msg(M_FATAL, "Uninitialising WFP failed!");
                 }
@@ -1877,7 +1877,7 @@ do_close_tun(struct context *c, bool force)
 #if defined(_WIN32)
             if (c->options.block_outside_dns)
             {
-                if (!win_wfp_uninit(c->options.msg_channel))
+                if (!win_wfp_uninit(adapter_index, c->options.msg_channel))
                 {
                     msg(M_FATAL, "Uninitialising WFP failed!");
                 }
src/openvpn/win32.c +27 -2
index 18e7aee..dc411fc 100644
@@ -61,6 +61,11 @@
 static HANDLE m_hEngineHandle = NULL; /* GLOBAL */
 
 /*
+ * TAP adapter original metric value
+ */
+static int tap_metric = -1; /* GLOBAL */
+
+/*
  * Windows internal socket API state (opaque).
  */
 static struct WSAData wsa_state; /* GLOBAL */
@@ -1337,6 +1342,21 @@ win_wfp_block_dns(const NET_IFINDEX index, const HANDLE msg_channel)
 
     status = add_block_dns_filters(&m_hEngineHandle, index, openvpnpath,
                                    block_dns_msg_handler);
+    if (status == 0)
+    {
+        tap_metric = get_interface_metric(index, AF_INET);
+        if (tap_metric < 0)
+        {
+            /* error, should not restore metric */
+            tap_metric = -1;
+        }
+        status = set_interface_metric(index, AF_INET, BLOCK_DNS_IFACE_METRIC);
+        if (!status)
+        {
+            set_interface_metric(index, AF_INET6, BLOCK_DNS_IFACE_METRIC);
+        }
+    }
+
     ret = (status == 0);
 
 out:
@@ -1345,19 +1365,24 @@ out:
 }
 
 bool
-win_wfp_uninit(const HANDLE msg_channel)
+win_wfp_uninit(const NET_IFINDEX index, const HANDLE msg_channel)
 {
     dmsg(D_LOW, "Uninitializing WFP");
 
     if (msg_channel)
     {
         msg(D_LOW, "Using service to delete block dns filters");
-        win_block_dns_service(false, -1, msg_channel);
+        win_block_dns_service(false, index, msg_channel);
     }
     else
     {
         delete_block_dns_filters(m_hEngineHandle);
         m_hEngineHandle = NULL;
+        if (tap_metric >= 0)
+        {
+            set_interface_metric(index, AF_INET, tap_metric);
+            set_interface_metric(index, AF_INET6, tap_metric);
+        }
     }
 
     return true;
src/openvpn/win32.h +1 -1
index 4ee44fd..cd1f101 100644
@@ -293,7 +293,7 @@ WCHAR *wide_string(const char *utf8, struct gc_arena *gc);
 
 bool win_wfp_block_dns(const NET_IFINDEX index, const HANDLE msg_channel);
 
-bool win_wfp_uninit(const HANDLE msg_channel);
+bool win_wfp_uninit(const NET_IFINDEX index, const HANDLE msg_channel);
 
 #define WIN_XP 0
 #define WIN_VISTA 1
src/openvpnserv/interactive.c +56 -5

98 lines not on this page · raw message

Signature · 1 line
2.9.3
#1 lore raw

Hi,

Quoted 6 lines · ValdikSS

Windows 10 before Creators Update used to resolve DNS using all available adapters and IP addresses in parallel.
Now it still resolves addresses using all available adapters but in a round-robin way, beginning with random adapter.

Hmm... If it starts with a random adapter, this metric lowering is not the right fix, isn't it? Or did you mean to say "starting from lowest metric adapter"?

Quoted 4 lines

This behaviour introduces significant delay when block-outside-dns is in use.
Fortunately, setting low metric for the TAP interface solves this issue, making Windows always pick with TAP adapter first.

Well, I'm looking for to excuses to avoid the hard work of a careful review :) If reducing metric is indeed the right approach in this cat and mouse game with MS, I'll try to look through this over the weekend.

Selva

author
#2 lore raw
Quoted 8 lines · Selva Nair

Hi,

On Thu, May 4, 2017 at 1:36 PM, ValdikSS <[email protected]... <mailto:[email protected]...>> wrote:

Hmm... If it starts with a random adapter, this metric lowering is not the right fix, isn't it? Or did you mean to say "starting from lowest metric adapter"?

Well, I'm looking for to excuses to avoid the hard work of a careful review :) If reducing metric is indeed the right approach in this cat and mouse game with MS, I'll try to look through this over the weekend.

From what I understand, Windows tries to use DNS in a round-robin fashion if adapter's metric is set to automatic, regardless of computed metric value. If we disable automatic metric on TAP adapter and set it lower than computed metric of other adapters, it would be used first. I could be not completely correct with this, but I tested it and it works.

Quoted 15 lines

Selva

------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

#3 lore raw
In reply to ValdikSS

Hi,

Please bear with me for making a few more comments. This close to final so only
a few minor issues.

Quoted 10 lines · ValdikSS

Windows 10 before Creators Update used to resolve DNS using all available adapters and IP addresses in parallel.
Now it still resolves addresses using all available adapters but in a round-robin way, beginning with random adapter.
This behaviour introduces significant delay when block-outside-dns is in use.
Fortunately, setting low metric for the TAP interface solves this issue, making Windows always pick with TAP adapter first.

Nitpicking here, but anyway a new version is required (see below), so it would be nice if you wrap lines in commit message to something like about 70 to 80 at most. Not sure whether we have a policy on this..

src/openvpn/block_dns.c +0 -0
Quoted 16 lines

index e31765e..9d49355 100644

src/openvpn/block_dns.c +6 -0
@@ -341,4 +341,79 @@ delete_block_dns_filters(HANDLE engine_handle)
     return err;
 }
+/*
+ * Returns interface metric value for specified interface index.
+ *
+ * Arguments:
+ *   index         : The index of TAP adapter.
+ *   family        : Address family (AF_INET for IPv4 and AF_INET6 for

IPv6).
+ * Returns positive metric value or zero for automatic metric on success,
+ * a less then zero error code on failure.
+ */

src/openvpn/block_dns.c

Quoted 7 lines
src/openvpn/block_dns.c +7 -0
+
+int
+get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family)
+{
+    DWORD err = 0;
+    MIB_IPINTERFACE_ROW ipiface;
+    InitializeIpInterfaceEntry(&ipiface);

InitializeIpInterfaceEntry() is missing in all but very recent mingw32 versions
(their commit logs show it was added in early 2015) so we may need to declare it in block_dns.c. I use Debian jessie (8.7) -- mingw gcc 4.9.1 and its
not there. I believe Samuli's build system uses an even older version.

Note that 64 bit build still succeeds with a warning but 32 bit will fail because
of stdcall. Yes, its stdcall though MSDN doesn't mention it as such.

src/openvpn/block_dns.c DWORD · line 45

Quoted 11 lines

typedef void (*block_dns_msg_handler_t) (DWORD err, const char *msg);

DWORD

src/openvpn/block_dns.c +2 -0
@@ -36,5 +39,32 @@ DWORD
 add_block_dns_filters(HANDLE *engine, int iface_index, const WCHAR
 *exe_path,
                       block_dns_msg_handler_t msg_handler_callback);
 
+/**
+ * Sets interface metric value for specified interface index
 

This would be "Get" or "Return" not "Set"

Quoted 13 lines
src/openvpn/block_dns.c +10 -0
+ *
+ * @param index The index of TAP adapter
+ * @param family Address family (AF_INET for IPv4 and AF_INET6 for IPv6)
+ *
+ * @return positive metric value or zero for automatic metric on success,
+ * a less then zero error code on failure.
+ */
+
+int
+get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY

family);

..

Quoted 17 lines
src/openvpn/win32.c +5 -0
index 18e7aee..dc411fc 100644
@@ -61,6 +61,11 @@
 static HANDLE m_hEngineHandle = NULL; /* GLOBAL */
 
 /*
+ * TAP adapter original metric value
+ */
+static int tap_metric = -1; /* GLOBAL */
+
+/*
  * Windows internal socket API state (opaque).
  */
 static struct WSAData wsa_state; /* GLOBAL */

src/openvpn/win32.c win_wfp_block_dns(const NET_IFINDEX index, const · line 1349

Quoted 8 lines
src/openvpn/win32.c +3 -0
@@ -1337,6 +1342,21 @@ win_wfp_block_dns(const NET_IFINDEX index, const
 HANDLE msg_channel)
 
     status = add_block_dns_filters(&m_hEngineHandle, index, openvpnpath,
                                    block_dns_msg_handler);
+    if (status == 0)
+    {
+        tap_metric = get_interface_metric(index, AF_INET);
 

The current metric is queried and saved only for v4 while the metric is set and restored for both v4 and v6. Isn't it necessary to also save the metric for both independently? I'm not sure whether anyone really uses different metric for v4 and v6 but Windows allows it.

Quoted 27 lines
src/openvpn/win32.c +12 -0
+        if (tap_metric < 0)
+        {
+            /* error, should not restore metric */
+            tap_metric = -1;
+        }
+        status = set_interface_metric(index, AF_INET,
 BLOCK_DNS_IFACE_METRIC);
+        if (!status)
+        {
+            set_interface_metric(index, AF_INET6, BLOCK_DNS_IFACE_METRIC);
+        }
+    }
+
     ret = (status == 0);

out:

src/openvpn/win32.c +1 -0
@@ -1325,6 +1368,7 @@ static VOID
 Undo(undo_lists_t *lists)
 {
     undo_type_t type;
+    block_dns_data_t *interface_data;
     for (type = 0; type < _undo_type_max; type++)
     {
         list_item_t **pnext = &(*lists)[type];

src/openvpn/win32.c Undo(undo_lists_t *lists) · line 1400

Quoted 9 lines
src/openvpn/win32.c +3 -2
@@ -1350,8 +1394,15 @@ Undo(undo_lists_t *lists)
                     break;
 
                 case block_dns:
-                    delete_block_dns_filters(item->data);
-                    item->data = NULL;
+                    interface_data = (block_dns_data_t*)(item->data);
+                    delete_block_dns_filters(interface_data->engine);
+                    if (interface_data->metric > 0)
 

Here ">" should be ">=0", else automatic metric is not restored.

Quoted 11 lines
src/openvpn/win32.c +6 -0
+                    {
+                        set_interface_metric(interface_data->index,
 AF_INET,
+                                             interface_data->metric);
+                        set_interface_metric(interface_data->index,
 AF_INET6,
+                                             interface_data->metric);
+                    }
                     break;
             }

Finally there are some trailing white space errors (6 places). This is a good
opportunity to fix those too.

Thanks,

Selva

author
#4 lore raw
Quoted 3 lines · Selva Nair

Hi,

Please bear with me for making a few more comments.

Please don't excuse. That's totally fine.

Quoted 8 lines

This close to final so only
a few minor issues.

On Thu, May 4, 2017 at 1:36 PM, ValdikSS <[email protected]... <mailto:[email protected]...>> wrote:

Nitpicking here, but anyway a new version is required (see below), so it would be nice if you wrap lines in commit message to something like about 70 to 80 at most. Not sure whether we have a policy on this..

Done.

Quoted 6 lines

InitializeIpInterfaceEntry() is missing in all but very recent mingw32 versions (their commit logs show it was added in early 2015) so we may need
to declare it in block_dns.c. I use Debian jessie (8.7) -- mingw gcc 4.9.1 and its not there. I believe Samuli's build system uses an even older version.

I don't know how to do that correctly. Can you please help me?

Quoted 6 lines

Note that 64 bit build still succeeds with a warning but 32 bit will fail because of stdcall. Yes, its stdcall though MSDN doesn't mention it as such.

This would be "Get" or "Return" not "Set"

Done.

Quoted 10 lines

..

The current metric is queried and saved only for v4 while the metric is set and restored for both v4 and v6. Isn't it necessary to also save the metric for both independently? I'm not sure whether anyone really uses different metric for v4 and v6 but Windows allows it.

Done.

Quoted 3 lines

Here ">" should be ">=0", else automatic metric is not restored.

Done.

Quoted 4 lines

Finally there are some trailing white space errors (6 places). This is a good opportunity to fix those too.

Done.

Please check
https://github.com/ValdikSS/openvpn-with-patches/commit/02058c8f043d2731011222a05686478a09e479ec

Text vesion:
https://github.com/ValdikSS/openvpn-with-patches/commit/02058c8f043d2731011222a05686478a09e479ec.patch

Quoted 15 lines

Thanks,

Selva

------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

#5 lore raw
Quoted 10 lines · ValdikSS

InitializeIpInterfaceEntry() is missing in all but very recent mingw32 versions
(their commit logs show it was added in early 2015) so we may need to declare it in block_dns.c. I use Debian jessie (8.7) -- mingw gcc 4.9.1 and its
not there. I believe Samuli's build system uses an even older version.

I don't know how to do that correctly. Can you please help me?

In block-dns.c where "WFP-related defines and GUIDs not in mingw32" are defined, add this

VOID NETIOAPI_API_ InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row );

Do a test build for 32 bit to see now this function compiles without warnings
and links without errors. 32 bit is a more stringent test because only in this
case the linker will fail to find stdcall functions without proper declarations.

That should be it.

Selva

author
#6 lore raw

Added it. Can't compile for 32 bit, I tried multiple times but can't wrestle down build system to compile my version from git. It never finds OpenSSL. It successfully compiles for 64 bit.

Please check v2.

Quoted 27 lines · Selva Nair

On Wed, May 10, 2017 at 12:08 PM, ValdikSS <[email protected]... <mailto:[email protected]...>> wrote:

In block-dns.c where "WFP-related defines and GUIDs not in mingw32" are defined, add this

VOID NETIOAPI_API_ InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row );

Do a test build for 32 bit to see now this function compiles without warnings and links without errors. 32 bit is a more stringent test because only in this case the linker will fail to find stdcall functions without proper declarations.

That should be it.

Selva

------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel