diff mbox series

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

Message ID 1516815105-17882-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v3] Use lowest metric interface when multiple interfaces match a route | expand

Commit Message

Selva Nair Jan. 24, 2018, 6:31 a.m. UTC 
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>

Signed-off-by: Selva Nair <selva.nair@gmail.com>

---
NOTE: depends on https://patchwork.openvpn.net/patch/136/

v3: Simpliyfy the patch using get_interface_metric from block_dns.c
    Simpler is also easier to review :)
    (requires patch 136 https://patchwork.openvpn.net/patch/136/)
v2:
    - Revert an unintented edit of route.c (a_index = ...)
    - Improve the commit message

 src/openvpn/route.c |  1 -
 src/openvpn/tun.c   | 17 +++++++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Jan Just Keijser Jan. 26, 2018, 2:02 a.m. UTC | #1 
Works as expected.
Tested-by: Jan Just Keijser <janjust@nikhef.nl>



On 24/01/18 18:31, selva.nair@gmail.com wrote:
> 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>
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
>
> ---
> NOTE: depends on https://patchwork.openvpn.net/patch/136/
>
> v3: Simpliyfy the patch using get_interface_metric from block_dns.c
>      Simpler is also easier to review :)
>      (requires patch 136 https://patchwork.openvpn.net/patch/136/)
> v2:
>      - Revert an unintented edit of route.c (a_index = ...)
>      - Improve the commit message
>
>   src/openvpn/route.c |  1 -
>   src/openvpn/tun.c   | 17 +++++++++++++++--
>   2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index f121d3f..218ca96 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -2785,7 +2785,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 2644d99..f424f82 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -45,6 +45,7 @@
>   #include "manage.h"
>   #include "route.h"
>   #include "win32.h"
> +#include "block_dns.h"
>   
>   #include "memdbg.h"
>   
> @@ -4480,6 +4481,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)
> @@ -4493,9 +4495,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, NULL);
>               if (first || hn > highest_netmask)
>               {
>                   highest_netmask = hn;
> +                if (metric >= 0)
> +                {
> +                    lowest_metric = metric;
> +                }
>                   if (count)
>                   {
>                       *count = 1;
> @@ -4509,16 +4516,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)
>       {


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jan Just Keijser Jan. 26, 2018, 2:11 a.m. UTC | #2 
the patch works as expected but I did notice something in the openvpn log :

Fri Jan 26 14:08:09 2018 do_ifconfig, tt->did_ifconfig_ipv6_setup=1
Fri Jan 26 14:08:10 2018 NETSH: C:\Windows\system32\netsh.exe interface 
ipv6 set address interface=17 2001:610:120::200:0:1001 store=active
Fri Jan 26 14:08:10 2018 add_route_ipv6(2001:610:120::200:0:0/112 -> 
2001:610:120::200:0:1001 metric 0) dev vpn0
Fri Jan 26 14:08:10 2018 C:\Windows\system32\netsh.exe interface ipv6 
add route 2001:610:120::200:0:0/112 interface=17 fe80::8 store=active
Fri Jan 26 14:08:10 2018 env_block: add 
PATH=C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem

the route was added with the default GW of fe80::8 : should I be worried ?
Note that this also happened with the regular 2.4.4 version of OpenVPN 
and also note that the TAP adapter on my Win7 laptop is named 'vpn0'

JJK


On 24-Jan-18 18:31, selva.nair@gmail.com wrote:
> 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>
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
>
> ---
> NOTE: depends on https://patchwork.openvpn.net/patch/136/
>
> v3: Simpliyfy the patch using get_interface_metric from block_dns.c
>      Simpler is also easier to review :)
>      (requires patch 136 https://patchwork.openvpn.net/patch/136/)
> v2:
>      - Revert an unintented edit of route.c (a_index = ...)
>      - Improve the commit message
>
>   src/openvpn/route.c |  1 -
>   src/openvpn/tun.c   | 17 +++++++++++++++--
>   2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index f121d3f..218ca96 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -2785,7 +2785,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 2644d99..f424f82 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -45,6 +45,7 @@
>   #include "manage.h"
>   #include "route.h"
>   #include "win32.h"
> +#include "block_dns.h"
>   
>   #include "memdbg.h"
>   
> @@ -4480,6 +4481,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)
> @@ -4493,9 +4495,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, NULL);
>               if (first || hn > highest_netmask)
>               {
>                   highest_netmask = hn;
> +                if (metric >= 0)
> +                {
> +                    lowest_metric = metric;
> +                }
>                   if (count)
>                   {
>                       *count = 1;
> @@ -4509,16 +4516,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)
>       {


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jan Just Keijser Jan. 26, 2018, 2:23 a.m. UTC | #3 
On 26/01/18 14:11, Jan Just Keijser wrote:
> the patch works as expected but I did notice something in the openvpn log :
>
> Fri Jan 26 14:08:09 2018 do_ifconfig, tt->did_ifconfig_ipv6_setup=1
> Fri Jan 26 14:08:10 2018 NETSH: C:\Windows\system32\netsh.exe interface ipv6 set address interface=17 2001:610:120::200:0:1001 
> store=active
> Fri Jan 26 14:08:10 2018 add_route_ipv6(2001:610:120::200:0:0/112 -> 2001:610:120::200:0:1001 metric 0) dev vpn0
> Fri Jan 26 14:08:10 2018 C:\Windows\system32\netsh.exe interface ipv6 add route 2001:610:120::200:0:0/112 interface=17 fe80::8 
> store=active
> Fri Jan 26 14:08:10 2018 env_block: add PATH=C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem
>
> the route was added with the default GW of fe80::8 : should I be worried ?
> Note that this also happened with the regular 2.4.4 version of OpenVPN and also note that the TAP adapter on my Win7 laptop is 
> named 'vpn0'
>
arrrgh, the important line is missing:
  ERROR: Windows route add ipv6 command failed: returned error code 1

sorry for the noise,

JJK




------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Jan. 26, 2018, 2:36 a.m. UTC | #4 
Hi,

On Fri, Jan 26, 2018 at 02:11:52PM +0100, Jan Just Keijser wrote:
> the route was added with the default GW of fe80::8 : should I be worried ?

fe80::8 is our/my tun-over-tap hack.

On "proper" tun devices, there is no ARP or IPv6 neighbour discovery, so
you can point routes toward the *interface* - or towards "an IP address
that the system knows is on the tun interface" (which we do for IPv4,
because that's how it was coded in the dark ages).  Since there is no
ARP, "a route towards tun" leads to "packet sent to tun fd, no further
processing, let openvpn deal with it".  Easy.

On TAP there is "ethernet underneath", so for each route destination,
an ARP or IPv6 neighbour discovery lookup needs to be done, and then
the packet is sent towards that MAC address.  Also fairly easy, since 
OpenVPN only deals with MAC address forwarding and lets the OSes on
both sides sort out the L2 discovery.


Now, tun-on-windows is "tun" as far as OpenVPN is concerned, and "TAP"
as far as Windows is concerned - so there needs to be an explicitely
named gateway address (v4 as v6), and ARP / IPv6 ND.  Since OpenVPN
doesn't deal with MAC stuff here, magic happens in the Windows tap 
driver - namely, if Windows sends an ARP request for "the ipv4 route 
gateway", the TAP driver answers it (openvpn tells it via ioctl() what
the gateway address is, so the TAP driver knows what addresses to 
listen for).  For IPv6, we have link-local, so I did not bother to 
implement a new ioctl() and logic on the OpenVPN side, but always set
the next-hop route to "fe80::8" - and the tap driver knows "oh, this
is magic, answer the ND query".


Long story short: 
 - for tap interfaces in tun mode, fe80::8 is what you want to see.  
 - For tap interfaces in *tap* mode, "a real IPv6 address of the gateway 
   on the other end" is what you want (ND goes back and forth over 
   OpenVPN/tap layer).

 - LAN routes (/128 bypass routes to the VPN server, for example) 
   should never use fe80::8 but the real link-local or global address
   of the gateway -> now with "it works if you have two interfaces!" :-)

 - your log file looks like it should :-)

gert
Selva Nair Jan. 26, 2018, 4:08 a.m. UTC | #5 
Hi,

On Fri, Jan 26, 2018 at 8:23 AM, Jan Just Keijser <janjust@nikhef.nl> wrote:
> On 26/01/18 14:11, Jan Just Keijser wrote:
>>
>> the patch works as expected but I did notice something in the openvpn log
>> :
>>
>> Fri Jan 26 14:08:09 2018 do_ifconfig, tt->did_ifconfig_ipv6_setup=1
>> Fri Jan 26 14:08:10 2018 NETSH: C:\Windows\system32\netsh.exe interface
>> ipv6 set address interface=17 2001:610:120::200:0:1001 store=active
>> Fri Jan 26 14:08:10 2018 add_route_ipv6(2001:610:120::200:0:0/112 ->
>> 2001:610:120::200:0:1001 metric 0) dev vpn0
>> Fri Jan 26 14:08:10 2018 C:\Windows\system32\netsh.exe interface ipv6 add
>> route 2001:610:120::200:0:0/112 interface=17 fe80::8 store=active
>> Fri Jan 26 14:08:10 2018 env_block: add
>> PATH=C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem
>>
>> the route was added with the default GW of fe80::8 : should I be worried ?
>> Note that this also happened with the regular 2.4.4 version of OpenVPN and
>> also note that the TAP adapter on my Win7 laptop is named 'vpn0'
>>
> arrrgh, the important line is missing:
>  ERROR: Windows route add ipv6 command failed: returned error code 1

Gert has explained the fe80::8 magic.

Here is my guess about the ERROR: The route already existed and so
errored trying to add again -- most likely from a previous run as it
seems we have a problem in deleting the route on exit (I guess this is
on Windows 10) (see Trac 1003:
https://community.openvpn.net/openvpn/ticket/1003 )

"netsh int ipv6 show route" will confirm that.

The route destinations have their host bits zeroed out before setting
route, but those bits reappear (which is strange)  in route delete.
And netsh doesnt like that at least on Windows 10. Gert, any idea why
this happens?

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jan Just Keijser Jan. 26, 2018, 4:20 a.m. UTC | #6 
Hi Selva,



On 26-Jan-18 16:08, Selva Nair wrote:
> On Fri, Jan 26, 2018 at 8:23 AM, Jan Just Keijser <janjust@nikhef.nl> wrote:
>> On 26/01/18 14:11, Jan Just Keijser wrote:
>>> the patch works as expected but I did notice something in the openvpn log
>>> :
>>>
>>> Fri Jan 26 14:08:09 2018 do_ifconfig, tt->did_ifconfig_ipv6_setup=1
>>> Fri Jan 26 14:08:10 2018 NETSH: C:\Windows\system32\netsh.exe interface
>>> ipv6 set address interface=17 2001:610:120::200:0:1001 store=active
>>> Fri Jan 26 14:08:10 2018 add_route_ipv6(2001:610:120::200:0:0/112 ->
>>> 2001:610:120::200:0:1001 metric 0) dev vpn0
>>> Fri Jan 26 14:08:10 2018 C:\Windows\system32\netsh.exe interface ipv6 add
>>> route 2001:610:120::200:0:0/112 interface=17 fe80::8 store=active
>>> Fri Jan 26 14:08:10 2018 env_block: add
>>> PATH=C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem
>>>
>>> the route was added with the default GW of fe80::8 : should I be worried ?
>>> Note that this also happened with the regular 2.4.4 version of OpenVPN and
>>> also note that the TAP adapter on my Win7 laptop is named 'vpn0'
>>>
>> arrrgh, the important line is missing:
>>   ERROR: Windows route add ipv6 command failed: returned error code 1
> Gert has explained the fe80::8 magic.
>
> Here is my guess about the ERROR: The route already existed and so
> errored trying to add again -- most likely from a previous run as it
> seems we have a problem in deleting the route on exit (I guess this is
> on Windows 10) (see Trac 1003:
> https://community.openvpn.net/openvpn/ticket/1003 )
>
> "netsh int ipv6 show route" will confirm that.
>
> The route destinations have their host bits zeroed out before setting
> route, but those bits reappear (which is strange)  in route delete.
> And netsh doesnt like that at least on Windows 10. Gert, any idea why
> this happens?
>
this is on *Windows 7*  and indeed that route is never deleted when 
OpenVPN exits. Here's the error associated with *that*:

Fri Jan 26 16:13:08 2018 ERROR: Windows route delete ipv6 command 
failed: returned error code 1
Fri Jan 26 16:13:09 2018 NETSH: C:\Windows\system32\netsh.exe interface 
ipv6 delete address vpn0 2001:610:120::200:0:1001 store=active
Fri Jan 26 16:13:09 2018 TAP: DHCP address released

C:\Users\janjust\OpenVPN\config> C:\Windows\system32\netsh.exe interface 
ipv6 delete route 2001:610:120::200:0:1001/112
interface=17 fe80::8 store=active
The parameter is incorrect.


HTH,

JJK


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair Jan. 26, 2018, 4:26 a.m. UTC | #7 
Hi,

On Fri, Jan 26, 2018 at 10:20 AM, Jan Just Keijser <janjust@nikhef.nl> wrote:
> Hi Selva,
>
>
>
>
> On 26-Jan-18 16:08, Selva Nair wrote:
>>
...

>>> arrrgh, the important line is missing:
>>>   ERROR: Windows route add ipv6 command failed: returned error code 1
>>
>> Gert has explained the fe80::8 magic.
>>
>> Here is my guess about the ERROR: The route already existed and so
>> errored trying to add again -- most likely from a previous run as it
>> seems we have a problem in deleting the route on exit (I guess this is
>> on Windows 10) (see Trac 1003:
>> https://community.openvpn.net/openvpn/ticket/1003 )
>>
>> "netsh int ipv6 show route" will confirm that.
>>
>> The route destinations have their host bits zeroed out before setting
>> route, but those bits reappear (which is strange)  in route delete.
>> And netsh doesnt like that at least on Windows 10. Gert, any idea why
>> this happens?
>>
> this is on *Windows 7*  and indeed that route is never deleted when OpenVPN
> exits. Here's the error associated with *that*:
>
> Fri Jan 26 16:13:08 2018 ERROR: Windows route delete ipv6 command failed:
> returned error code 1
> Fri Jan 26 16:13:09 2018 NETSH: C:\Windows\system32\netsh.exe interface ipv6
> delete address vpn0 2001:610:120::200:0:1001 store=active
> Fri Jan 26 16:13:09 2018 TAP: DHCP address released
>
> C:\Users\janjust\OpenVPN\config> C:\Windows\system32\netsh.exe interface
> ipv6 delete route 2001:610:120::200:0:1001/112
> interface=17 fe80::8 store=active
> The parameter is incorrect.

The error is due to the host part (1001) in the route destination --
changing that to
C:\Windows\system32\netsh.exe interface  ipv6 delete route
2001:610:120::200:0:/112
should succeed.

The mystery (at least for me)  is where that host part is coming
from... Its zeroed out before setting the route, and I thought the
same (?) route list pointer is
passed in while deleting routes.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jan Just Keijser Jan. 26, 2018, 4:33 a.m. UTC | #8 
Hi,

On 26/01/18 16:26, Selva Nair wrote:
> On Fri, Jan 26, 2018 at 10:20 AM, Jan Just Keijser <janjust@nikhef.nl> wrote:
>>
>> On 26-Jan-18 16:08, Selva Nair wrote:
>>
>>>> arrrgh, the important line is missing:
>>>>    ERROR: Windows route add ipv6 command failed: returned error code 1
>>> Gert has explained the fe80::8 magic.
>>>
>>> Here is my guess about the ERROR: The route already existed and so
>>> errored trying to add again -- most likely from a previous run as it
>>> seems we have a problem in deleting the route on exit (I guess this is
>>> on Windows 10) (see Trac 1003:
>>> https://community.openvpn.net/openvpn/ticket/1003 )
>>>
>>> "netsh int ipv6 show route" will confirm that.
>>>
>>> The route destinations have their host bits zeroed out before setting
>>> route, but those bits reappear (which is strange)  in route delete.
>>> And netsh doesnt like that at least on Windows 10. Gert, any idea why
>>> this happens?
>>>
>> this is on *Windows 7*  and indeed that route is never deleted when OpenVPN
>> exits. Here's the error associated with *that*:
>>
>> Fri Jan 26 16:13:08 2018 ERROR: Windows route delete ipv6 command failed:
>> returned error code 1
>> Fri Jan 26 16:13:09 2018 NETSH: C:\Windows\system32\netsh.exe interface ipv6
>> delete address vpn0 2001:610:120::200:0:1001 store=active
>> Fri Jan 26 16:13:09 2018 TAP: DHCP address released
>>
>> C:\Users\janjust\OpenVPN\config> C:\Windows\system32\netsh.exe interface
>> ipv6 delete route 2001:610:120::200:0:1001/112
>> interface=17 fe80::8 store=active
>> The parameter is incorrect.
> The error is due to the host part (1001) in the route destination --
> changing that to
> C:\Windows\system32\netsh.exe interface  ipv6 delete route
> 2001:610:120::200:0:/112
> should succeed.
>
> The mystery (at least for me)  is where that host part is coming
> from... Its zeroed out before setting the route, and I thought the
> same (?) route list pointer is passed in while deleting routes.
>
>
that also did not work; what does work is:
   C:\Windows\system32\netsh.exe interface ipv6 delete route 2001:610:120::200:0:0/112

so indeed, the host part is added at deletion time.

HTH,

JJK


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Jan. 26, 2018, 4:35 a.m. UTC | #9 
Hi,

On Fri, Jan 26, 2018 at 10:26:58AM -0500, Selva Nair wrote:
> The mystery (at least for me)  is where that host part is coming
> from... Its zeroed out before setting the route, and I thought the
> same (?) route list pointer is
> passed in while deleting routes.

"I seem to remember code changes related to that, possibly related to
the introduction of the interactive service handle"

Can't look right now, but I think we had 2x zeroing and that was 
unified and stored, or something like that... need to check, later
this evening.

gert
Gert Doering Feb. 20, 2018, 1:58 a.m. UTC | #10 
Acked-by: Gert Doering <gert@greenie.muc.de>

Based on JJK's test results, staring at the code, understanding the
intent :-) and test compilation on 16.04.

Your patch has been applied to the master and release/2.4 branch.

TODO: figuring out what's wrong with the IPv6 route "host bits", but
that's totally outside these code paths, it just came up in this thread.

commit 3854d4040e0d6fd2a58292e8bb1c1fbae5c17bb1 (master)
commit 965c7906f61e6de30adc1cc2d24ec73e345e0007 (release/2.4)
Author: Selva Nair
Date:   Wed Jan 24 12:31:45 2018 -0500

     Use lowest metric interface when multiple interfaces match a route

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <1516815105-17882-1-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16347.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Feb. 20, 2018, 3:35 a.m. UTC | #11 
Hi,

On Fri, Jan 26, 2018 at 10:26:58AM -0500, Selva Nair wrote:
> The mystery (at least for me)  is where that host part is coming
> from... Its zeroed out before setting the route, and I thought the
> same (?) route list pointer is
> passed in while deleting routes.

So.  Staring a bit at the code (while sf.net is refusing to forward my
last mail to patchwork... so I can't go ahead and commit & close more
patches).

The clearing of the route happens in 

route.c: route_ipv6_clear_host_bits()

which is only called from add_route_ipv6().

route_ipv6_clear_host_bits() does clear the "call by reference" route_ipv6 
structure, though, so it really should bubble up the call chain to
add_routes(), zeroeing out the bits in the "rl6->routes_ipv6" IPv6 route
list.

Testing this on linux with "--route-ipv6 2001:db8::1234:abcd/112" I can 
see that this seems to work correctly...

Tue Feb 20 15:19:02 2018 add_route_ipv6(2001:db8::1234:0/112 -> fd00:abcd:194:4::1 metric -1) dev tap3
Tue Feb 20 15:19:02 2018 /bin/route -A inet6 add 2001:db8::1234:0/112 dev tap3 gw fd00:abcd:194:4::1

Tue Feb 20 15:19:15 2018 delete_route_ipv6(2001:db8::1234:0/112)
Tue Feb 20 15:19:15 2018 /bin/route -A inet6 del 2001:db8::1234:0/112 dev tap3 gw fd00:abcd:194:4::1

(just grabbing a random test config, "tap" has no significance here, works 
the same with "tun")


"master" behaves the same here as release/2.4.


Re-reading the thread, I can now see where this is coming from - this 
is not "normal routes" but "on-link interface routes", and this is 
something that got broken on introducing the iservice (where the zeroeing
of the host bits was changed from "zap the local copy" to "zap the 
upstream copy, and do not zap it in delete_route() again, because, not 
needed") - commit a24dd2e31.


The on-link route is installed by tun.c:add_route_connected_v6_net(),
which operates on a on-stack instance of "struct route_ipv6"...

void
add_route_connected_v6_net(struct tuntap *tt,
                           const struct env_set *es)
{
    struct route_ipv6 r6;

    CLEAR(r6);
    r6.network = tt->local_ipv6;
    r6.netbits = tt->netbits_ipv6;
    r6.gateway = tt->local_ipv6;
    r6.metric  = 0;                     /* connected route */
    r6.flags   = RT_DEFINED | RT_METRIC_DEFINED;
    add_route_ipv6(&r6, tt, 0, es);
}

... so the zero'ing in add_route_ipv6() does not "stick".

On termination, we call delete_route_connected_v6_net(), which sets up
a new "route_ipv6" copy... with all bits set.


Jan Just, could you please test the following patch?  This will explicitly
clear the host bits for the "on-link" route again.

Fully untested :-)

gert


diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index ebb15bd3..79453665 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -839,6 +839,7 @@ delete_route_connected_v6_net(struct tuntap *tt,
     r6.gateway = tt->local_ipv6;
     r6.metric  = 0;                     /* connected route */
     r6.flags   = RT_DEFINED | RT_ADDED | RT_METRIC_DEFINED;
+    route_ipv6_clear_host_bits(&r6);
     delete_route_ipv6(&r6, tt, 0, es);
 }
 #endif /* if defined(_WIN32) || defined(TARGET_DARWIN) || defined(TARGET_NETBSD) || defined(TARGET_OPENBSD) */
Gert Doering Feb. 20, 2018, 4:48 a.m. UTC | #12 
Hi,

On Tue, Feb 20, 2018 at 03:35:02PM +0100, Gert Doering wrote:
> Jan Just, could you please test the following patch?  This will explicitly
> clear the host bits for the "on-link" route again.
> 
> Fully untested :-)

And indeed, it does not link, because route_ipv6_clear_host_bits() isn't
exported from route.c

A proper patch that is actually compiling and has been tested on Win7
will be coming soon (in a new thread, so patchwork can properly pick 
it up).

Win7 seems to ignore the extra host bits on route removal, so I cannot
"see" the actual *error* - but I can see whether or not the netsh.exe
invocation is correct, which is good enough to test whether the fix
is working.

gert
diff mbox series

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index f121d3f..218ca96 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2785,7 +2785,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 2644d99..f424f82 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -45,6 +45,7 @@ 
 #include "manage.h"
 #include "route.h"
 #include "win32.h"
+#include "block_dns.h"
 
 #include "memdbg.h"
 
@@ -4480,6 +4481,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)
@@ -4493,9 +4495,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, NULL);
             if (first || hn > highest_netmask)
             {
                 highest_netmask = hn;
+                if (metric >= 0)
+                {
+                    lowest_metric = metric;
+                }
                 if (count)
                 {
                     *count = 1;
@@ -4509,16 +4516,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)
     {