[Openvpn-devel,v4] route.c: use sitnl to implement get_default_gateway_ipv6()

Message ID 20190708224806.19887-1-a@unstable.cc
State Superseded
Headers show
Series [Openvpn-devel,v4] route.c: use sitnl to implement get_default_gateway_ipv6() | expand

Commit Message

Antonio Quartulli July 8, 2019, 12:48 p.m. UTC
From: Antonio Quartulli <antonio@openvpn.net>

get_default_gateway_ipv6() has always been implemented using
netlink, however, now that we have sitnl, we can re-use the
latter and get rid of the netlink code from route.c.

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---

Changes from v1:
- use IN6_IS_ADDR_UNSPECIFIED to check if GW address is specified. This fixes
  the issue of never setting the ON_LINK flag for IPv6 routes


 src/openvpn/init.c                |   9 +-
 src/openvpn/networking_iproute2.c |   2 +-
 src/openvpn/options.c             |   2 +-
 src/openvpn/route.c               | 163 ++++--------------------------
 src/openvpn/route.h               |   7 +-
 5 files changed, 31 insertions(+), 152 deletions(-)

Comments

Gert Doering July 14, 2019, 10:02 a.m. UTC | #1
Hi,

On Tue, Jul 09, 2019 at 12:48:06AM +0200, Antonio Quartulli wrote:
> 
> Changes from v1:
> - use IN6_IS_ADDR_UNSPECIFIED to check if GW address is specified. This fixes
>   the issue of never setting the ON_LINK flag for IPv6 routes

Something is still not working correctly.

It *must not* set the ON_LINK flag for routes that are behind a gatway
("route to gateway" vs. "route to interface"), but it still does.

Here's what I tested:

 - my home network has 2001:608:4:0::/64 on link
 - I have set up a route fd00:eeee:200::/48 -> tun0  (no gateway)
 - default gateway is either statically configured (FreeBSD) or learned
   from RA (linux)
 - I asked openvpn the following question:

	openvpn --show-gateway
	openvpn --show-gateway 2001:608::2
	openvpn --show-gateway 2001:608:4::123
	openvpn --show-gateway fd00:eeee:200::1

    the first is "find me the way to reach ::", which is via default 
    gateway (and the target is not ON_LINK), the second is "find me the
    way to this particular IPv6 address" - which is also via default
    gateway (here), and not ON_LINK.

    The third is "find me the way to reach 2001:608:4::123", which is
    another host on the LAN, so packets for this IPv6 address must be 
    sent "towards the LAN" not "to a gateway" -> this is ON_LINK.

    The fourth is something of an oddball, point to point interfaces
    do not have neighbour discovery - so all routes to a gateway *behind*
    a p2p interface are also "ON_LINK", as there is no gateway we could
    put into a route.


Now, this is what OpenVPN 2.4.7 prints here (or master "with no patch")
(leaving off IPv4):

$ openvpn247 --show-gateway
Sun Jul 14 21:51:09 2019 ROUTE6_GATEWAY fe80::250:43ff:fe01:dc37 IFACE=enp0s25
$ openvpn247 --show-gateway 2001:608::2
Sun Jul 14 21:51:22 2019 ROUTE6_GATEWAY fe80::250:43ff:fe01:dc37 IFACE=enp0s25
$ openvpn247 --show-gateway 2001:608:4::123
Sun Jul 14 21:51:33 2019 ROUTE6_GATEWAY 2001:608:4::123 ON_LINK IFACE=enp0s25
$ openvpn247 --show-gateway  fd00:eeee:200::1
Sun Jul 14 21:52:14 2019 ROUTE6_GATEWAY fd00:eeee:200::1 ON_LINK IFACE=tun0

and this is what OpenVPN master + your patch prints

$ src/openvpn/openvpn --show-gateway
Sun Jul 14 21:42:25 2019 ROUTE6_GATEWAY fe80::250:43ff:fe01:dc37 ON_LINK IFACE=enp0s25 HWADDR=00:00:00:00:54:97
$ src/openvpn/openvpn --show-gateway 2001:608::2     
Sun Jul 14 21:46:02 2019 ROUTE6_GATEWAY fe80::250:43ff:fe01:dc37 ON_LINK IFACE=enp0s25 HWADDR=00:00:00:00:54:b7
$ src/openvpn/openvpn --show-gateway 2001:608:4::123
Sun Jul 14 21:42:51 2019 ROUTE6_GATEWAY 2001:608:4::123 ON_LINK IFACE=enp0s25
$ src/openvpn/openvpn --show-gateway fd00:eeee:200::1
Sun Jul 14 21:42:59 2019 ROUTE6_GATEWAY fd00:eeee:200::1 ON_LINK IFACE=tun0 HWADDR=00:00:00:00:54:87

as you can see, it now insists on "everything is ON_LINK".


Now... I think the issue might be just that you removed the

-    CLEAR(*rgi6);

bit, so "there is cruft in that structure, and it is flagged as 
'the ON_LINK bit is set'" - but it's too late for me to do a full
code review, so can't say for sure.  If I just add it back, it
behaves correctly, but it would be good to give this function another
long and hard stare...

However - as it is today, I need to NAK this.  Sorry.  (And I should
have been much quicker)

gert

(PS: FreeBSD is, as expected, showing the same things before/after, with
ON_LINK only for test 3+4 - but that's a totally different code path, so
no surprises here)
Antonio Quartulli July 15, 2019, 4:45 a.m. UTC | #2
Hi,

On 14/07/2019 22:02, Gert Doering wrote:
> It *must not* set the ON_LINK flag for routes that are behind a gatway
> ("route to gateway" vs. "route to interface"), but it still does.
> 

[SKIP SKIP]

> 
> as you can see, it now insists on "everything is ON_LINK".
> 

interesting to note that on my test machine I don't see this behaviour.
ON_LINK is returned only when appropriate.

> 
> Now... I think the issue might be just that you removed the
> 
> -    CLEAR(*rgi6);
> 
> bit, so "there is cruft in that structure, and it is flagged as 
> 'the ON_LINK bit is set'" - but it's too late for me to do a full
> code review, so can't say for sure.  If I just add it back, it
> behaves correctly, but it would be good to give this function another
> long and hard stare...
> 

This makes a lot of sense: that CLEAR() statement has been removed by
accident.

And honestly this missing initialization is somewhat compatible with not
always seeing the problem (initialization of stack variables may differ
on different platforms based on compiler version, kernel options, etc..).

Actually I also realized that the 'gw' variable introduced next to this
line was not used, so I have got rid of it.

> However - as it is today, I need to NAK this.  Sorry.  (And I should
> have been much quicker)

No worries. v5 incoming!!

Cheers,

> 
> gert
> 
> (PS: FreeBSD is, as expected, showing the same things before/after, with
> ON_LINK only for test 3+4 - but that's a totally different code path, so
> no surprises here)
>

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 87976290..5ccf4d9d 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1486,7 +1486,8 @@  static void
 do_init_route_ipv6_list(const struct options *options,
                         struct route_ipv6_list *route_ipv6_list,
                         const struct link_socket_info *link_socket_info,
-                        struct env_set *es)
+                        struct env_set *es,
+                        openvpn_net_ctx_t *ctx)
 {
     const char *gw = NULL;
     int metric = -1;            /* no metric set */
@@ -1522,7 +1523,8 @@  do_init_route_ipv6_list(const struct options *options,
                              gw,
                              metric,
                              link_socket_current_remote_ipv6(link_socket_info),
-                             es))
+                             es,
+                             ctx))
     {
         /* copy routes to environment */
         setenv_routes_ipv6(es, route_ipv6_list);
@@ -1782,7 +1784,8 @@  do_open_tun(struct context *c)
     if (c->options.routes_ipv6 && c->c1.route_ipv6_list)
     {
         do_init_route_ipv6_list(&c->options, c->c1.route_ipv6_list,
-                                &c->c2.link_socket->info, c->c2.es);
+                                &c->c2.link_socket->info, c->c2.es,
+                                &c->net_ctx);
     }
 
     /* do ifconfig */
diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c
index 918d62ef..a5824306 100644
--- a/src/openvpn/networking_iproute2.c
+++ b/src/openvpn/networking_iproute2.c
@@ -375,7 +375,7 @@  net_route_v4_best_gw(openvpn_net_ctx_t *ctx, const in_addr_t *dst,
 
 /*
  * The following function is not implemented in the iproute backend as it
- * already uses netlink in route.c.
+ * uses the sitnl implementation from networking_sitnl.c.
  *
  * int
  * net_route_v6_best_gw(const struct in6_addr *dst, int prefixlen,
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index a37b7146..2d3865a6 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5012,7 +5012,7 @@  add_option(struct options *options,
         }
         net_ctx_init(NULL, &net_ctx);
         get_default_gateway(&rgi, &net_ctx);
-        get_default_gateway_ipv6(&rgi6, &remote);
+        get_default_gateway_ipv6(&rgi6, &remote, &net_ctx);
         print_default_gateway(M_INFO, &rgi, &rgi6);
         openvpn_exit(OPENVPN_EXIT_STATUS_GOOD); /* exit point */
     }
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 4cdc4a9f..a9071dff 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -765,7 +765,8 @@  init_route_ipv6_list(struct route_ipv6_list *rl6,
                      const char *remote_endpoint,
                      int default_metric,
                      const struct in6_addr *remote_host_ipv6,
-                     struct env_set *es)
+                     struct env_set *es,
+                     openvpn_net_ctx_t *ctx)
 {
     struct gc_arena gc = gc_new();
     bool ret = true;
@@ -790,7 +791,7 @@  init_route_ipv6_list(struct route_ipv6_list *rl6,
     msg(D_ROUTE, "GDG6: remote_host_ipv6=%s",
         remote_host_ipv6 ?  print_in6_addr(*remote_host_ipv6, 0, &gc) : "n/a" );
 
-    get_default_gateway_ipv6(&rl6->rgi6, remote_host_ipv6);
+    get_default_gateway_ipv6(&rl6->rgi6, remote_host_ipv6, ctx);
     if (rl6->rgi6.flags & RGI_ADDR_DEFINED)
     {
         setenv_str(es, "net_gateway_ipv6", print_in6_addr(rl6->rgi6.gateway.addr_ipv6, 0, &gc));
@@ -2799,7 +2800,7 @@  windows_route_find_if_index(const struct route_ipv4 *r, const struct tuntap *tt)
  */
 void
 get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
-                         const struct in6_addr *dest)
+                         const struct in6_addr *dest, openvpn_net_ctx_t *ctx)
 {
     struct gc_arena gc = gc_new();
     MIB_IPFORWARD_ROW2 BestRoute;
@@ -3323,152 +3324,30 @@  struct rtreq {
 
 void
 get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
-                         const struct in6_addr *dest)
+                         const struct in6_addr *dest, openvpn_net_ctx_t *ctx)
 {
-    int nls = -1;
-    struct rtreq rtreq;
-    struct rtattr *rta;
+    struct in_addr gw;
+    int flags;
 
-    char rtbuf[2000];
-    ssize_t ssize;
+    CLEAR(gw);
 
-    CLEAR(*rgi6);
-
-    nls = socket( PF_NETLINK, SOCK_RAW, NETLINK_ROUTE );
-    if (nls < 0)
-    {
-        msg(M_WARN|M_ERRNO, "GDG6: socket() failed" ); goto done;
-    }
-
-    /* bind() is not needed, no unsolicited msgs coming in */
-
-    /* request best matching route, see netlink(7) for explanations
-     */
-    CLEAR(rtreq);
-    rtreq.nh.nlmsg_type = RTM_GETROUTE;
-    rtreq.nh.nlmsg_flags = NLM_F_REQUEST;       /* best match only */
-    rtreq.rtm.rtm_family = AF_INET6;
-    rtreq.rtm.rtm_src_len = 0;                  /* not source dependent */
-    rtreq.rtm.rtm_dst_len = 128;                /* exact dst */
-    rtreq.rtm.rtm_table = RT_TABLE_MAIN;
-    rtreq.rtm.rtm_protocol = RTPROT_UNSPEC;
-    rtreq.nh.nlmsg_len = NLMSG_SPACE(sizeof(rtreq.rtm));
-
-    /* set RTA_DST for target IPv6 address we want */
-    rta = (struct rtattr *)(((char *) &rtreq)+NLMSG_ALIGN(rtreq.nh.nlmsg_len));
-    rta->rta_type = RTA_DST;
-    rta->rta_len = RTA_LENGTH(16);
-    rtreq.nh.nlmsg_len = NLMSG_ALIGN(rtreq.nh.nlmsg_len)
-                         +RTA_LENGTH(16);
-
-    if (dest == NULL)                   /* ::, unspecified */
-    {
-        memset( RTA_DATA(rta), 0, 16 ); /* :: = all-zero */
-    }
-    else
-    {
-        memcpy( RTA_DATA(rta), (void *)dest, 16 );
-    }
-
-    /* send and receive reply */
-    if (send( nls, &rtreq, rtreq.nh.nlmsg_len, 0 ) < 0)
-    {
-        msg(M_WARN|M_ERRNO, "GDG6: send() failed" ); goto done;
-    }
-
-    ssize = recv(nls, rtbuf, sizeof(rtbuf), MSG_TRUNC);
-
-    if (ssize < 0)
+    if (net_route_v6_best_gw(ctx, dest, 0, &rgi6->gateway.addr_ipv6,
+                             rgi6->iface) == 0)
     {
-        msg(M_WARN|M_ERRNO, "GDG6: recv() failed" ); goto done;
-    }
-
-    if (ssize > sizeof(rtbuf))
-    {
-        msg(M_WARN, "get_default_gateway_ipv6: returned message too big for buffer (%d>%d)", (int)ssize, (int)sizeof(rtbuf) );
-        goto done;
-    }
-
-    struct nlmsghdr *nh;
-
-    for (nh = (struct nlmsghdr *)rtbuf;
-         NLMSG_OK(nh, ssize);
-         nh = NLMSG_NEXT(nh, ssize))
-    {
-        struct rtmsg *rtm;
-        int attrlen;
-
-        if (nh->nlmsg_type == NLMSG_DONE)
-        {
-            break;
-        }
-
-        if (nh->nlmsg_type == NLMSG_ERROR)
+        if (!IN6_IS_ADDR_UNSPECIFIED(rgi6->gateway.addr_ipv6.s6_addr))
         {
-            struct nlmsgerr *ne = (struct nlmsgerr *)NLMSG_DATA(nh);
-
-            /* since linux-4.11 -ENETUNREACH is returned when no route can be
-             * found. Don't print any error message in this case */
-            if (ne->error != -ENETUNREACH)
-            {
-                msg(M_WARN, "GDG6: NLMSG_ERROR: error %s\n",
-                    strerror(-ne->error));
-            }
-            break;
+            rgi6->flags |= RGI_ADDR_DEFINED;
         }
 
-        if (nh->nlmsg_type != RTM_NEWROUTE)
+        if (rgi6->iface)
         {
-            /* shouldn't happen */
-            msg(M_WARN, "GDG6: unexpected msg_type %d", nh->nlmsg_type );
-            continue;
-        }
-
-        rtm = (struct rtmsg *)NLMSG_DATA(nh);
-        attrlen = RTM_PAYLOAD(nh);
-
-        /* we're only looking for routes in the main table, as "we have
-         * no IPv6" will lead to a lookup result in "Local" (::/0 reject)
-         */
-        if (rtm->rtm_family != AF_INET6
-            || rtm->rtm_table != RT_TABLE_MAIN)
-        {
-            continue;
-        }                               /* we're not interested */
-
-        for (rta = RTM_RTA(rtm);
-             RTA_OK(rta, attrlen);
-             rta = RTA_NEXT(rta, attrlen))
-        {
-            if (rta->rta_type == RTA_GATEWAY)
-            {
-                if (RTA_PAYLOAD(rta) != sizeof(struct in6_addr) )
-                {
-                    msg(M_WARN, "GDG6: RTA_GW size mismatch"); continue;
-                }
-                rgi6->gateway.addr_ipv6 = *(struct in6_addr *) RTA_DATA(rta);
-                rgi6->flags |= RGI_ADDR_DEFINED;
-            }
-            else if (rta->rta_type == RTA_OIF)
-            {
-                char ifname[IF_NAMESIZE+1];
-                int oif;
-                if (RTA_PAYLOAD(rta) != sizeof(oif) )
-                {
-                    msg(M_WARN, "GDG6: oif size mismatch"); continue;
-                }
-
-                memcpy(&oif, RTA_DATA(rta), sizeof(oif));
-                if_indextoname(oif,ifname);
-                strncpy( rgi6->iface, ifname, sizeof(rgi6->iface)-1 );
-                rgi6->flags |= RGI_IFACE_DEFINED;
-            }
+            rgi6->flags |= RGI_IFACE_DEFINED;
         }
     }
 
     /* if we have an interface but no gateway, the destination is on-link */
-    if ( ( rgi6->flags & (RGI_IFACE_DEFINED|RGI_ADDR_DEFINED) ) ==
-         RGI_IFACE_DEFINED)
+    flags = rgi6->flags & (RGI_IFACE_DEFINED | RGI_ADDR_DEFINED);
+    if (flags == RGI_IFACE_DEFINED)
     {
         rgi6->flags |= (RGI_ADDR_DEFINED | RGI_ON_LINK);
         if (dest)
@@ -3476,12 +3355,6 @@  get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
             rgi6->gateway.addr_ipv6 = *dest;
         }
     }
-
-done:
-    if (nls >= 0)
-    {
-        close(nls);
-    }
 }
 
 #elif defined(TARGET_DARWIN) || defined(TARGET_SOLARIS)    \
@@ -3758,7 +3631,7 @@  done:
 
 void
 get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
-                         const struct in6_addr *dest)
+                         const struct in6_addr *dest, openvpn_net_ctx_t *ctx)
 {
 
     struct rtmsg m_rtmsg;
@@ -3944,7 +3817,7 @@  get_default_gateway(struct route_gateway_info *rgi, openvpn_net_ctx_t *ctx)
 }
 void
 get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
-                         const struct in6_addr *dest)
+                         const struct in6_addr *dest, openvpn_net_ctx_t *ctx)
 {
     msg(D_ROUTE, "no support for get_default_gateway_ipv6() on this system");
     CLEAR(*rgi6);
diff --git a/src/openvpn/route.h b/src/openvpn/route.h
index e552e6ec..31d38e36 100644
--- a/src/openvpn/route.h
+++ b/src/openvpn/route.h
@@ -31,6 +31,7 @@ 
 #include "basic.h"
 #include "tun.h"
 #include "misc.h"
+#include "networking.h"
 
 #ifdef _WIN32
 /*
@@ -291,7 +292,8 @@  bool init_route_ipv6_list(struct route_ipv6_list *rl6,
                           const char *remote_endpoint,
                           int default_metric,
                           const struct in6_addr *remote_host,
-                          struct env_set *es);
+                          struct env_set *es,
+                          openvpn_net_ctx_t *ctx);
 
 void route_list_add_vpn_gateway(struct route_list *rl,
                                 struct env_set *es,
@@ -323,7 +325,8 @@  void get_default_gateway(struct route_gateway_info *rgi,
                          openvpn_net_ctx_t *ctx);
 
 void get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi,
-                              const struct in6_addr *dest);
+                              const struct in6_addr *dest,
+                              openvpn_net_ctx_t *ctx);
 
 void print_default_gateway(const int msglevel,
                            const struct route_gateway_info *rgi,