[Openvpn-devel,v3,5/7] route.c: use sitnl to implement get_default_gateway_ipv6()

Message ID 20181219050118.6568-6-a@unstable.cc
State Accepted
Headers show
Series introduce networking API and add netlink support for Linux | expand

Commit Message

Antonio Quartulli Dec. 18, 2018, 6:01 p.m. UTC
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 <a@unstable.cc>
---
 src/openvpn/init.c                |   9 +-
 src/openvpn/networking_iproute2.c |   2 +-
 src/openvpn/options.c             |   2 +-
 src/openvpn/route.c               | 161 ++++--------------------------
 src/openvpn/route.h               |   7 +-
 5 files changed, 30 insertions(+), 151 deletions(-)

Comments

Arne Schwabe May 10, 2019, 2:23 a.m. UTC | #1
Am 19.12.18 um 06:01 schrieb Antonio Quartulli:
> 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.
> 

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering June 5, 2019, 12:34 a.m. UTC | #2
Unfortunately, I need to NAK this, because this will break some people's
use of OpenVPN in an IPv6 context (namely, where the OpenVPN server is
sitting in the same LAN as the client - think "EduRoam WiFi").


I did the same thing as before - built an iproute2 binary and a sitnl
binary, and tested t_client plus some manual commands.

Under some conditions, the output is *differen*, and this is very bad.

Here's the output of "openvpn --show-gateway" for my local network...

iproute2 build:
Wed Jun  5 12:04:53 2019 ROUTE6_GATEWAY fe80::250:43ff:fe01:dc37 IFACE=enp0s25

sitnl build:
Wed Jun  5 12:04:56 2019 ROUTE6_GATEWAY fe80::250:43ff:fe01:dc37 ON_LINK IFACE=enp0s25

which I find surprising, since they should both use the same backend
now...?  "Older binaries" print the gateway without ON_LINK.

It gets weirder... if I ask for something which actually *is* on-link
(another address out of my on-link subnet), I get the same thing:

$ src/openvpn/openvpn --show-gateway 2001:608:4::1

iproute2 build:
Wed Jun  5 12:19:37 2019 ROUTE6_GATEWAY :: IFACE=enp0s25

sitnl build:
Wed Jun  5 12:18:20 2019 ROUTE6_GATEWAY :: ON_LINK IFACE=enp0s25 HWADDR=00:00:00:00:54:57

and *sometimes*
Wed Jun  5 12:19:56 2019 ROUTE6_GATEWAY :: IFACE=enp0s25


while my old code alwyays prints

Wed Jun  5 12:24:51 2019 ROUTE6_GATEWAY 2001:608:4::1 ON_LINK IFACE=enp0s25

("what you are looking for is on this link and it is the gateway").



This is a regression and needs to be fixed -> so, NAK.  

On-Link targets always need to have ON_LINK set for both builds, off-link 
targets (if there exists a default-gateway) must never have it set, and 
the gateway address should.

I have no idea what is going wrong here, because both implementations
should behave exactly the same, since they use the same backend - but
this smells like "something is not correct, and it depends on build
or run-time factors".  Maybe something isn't properly initialized 
when rewriting this...  the bug is not in code that is introduced
by this patch (it's hiding in networking_sitnl.c, I assume) but this
patch is what activates the code path, 


A few comments on the actual code changes :-) - the introduction of 
*ctx here cannot be avoided.

This strikes me as a bit funky, though...

+    struct in_addr gw;
+    CLEAR(gw);

.. and then "gw" is not used at all?

The introduction of a "flags" temporary variable can be certainly be
argued about (I find it less readable for a one-shot comparison), but 
if you do, why not define it where it's used (C99)?

While at it, please kick out the definition of "struct rtreq" in
route.c, right above the TARGET_LINUX get_default_gateway_ipv6() - no
longer needed since we do not do the netlink stuff here anymore.


thanks,

gert

--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 395f4ff0..c036bdae 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1463,7 +1463,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 */
@@ -1499,7 +1500,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);
@@ -1759,7 +1761,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 1f890f45..8a9c67f9 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5014,7 +5014,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 483b46ae..60ea4fab 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));
@@ -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;
-
-    char rtbuf[2000];
-    ssize_t ssize;
-
-    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 );
-    }
+    struct in_addr gw;
+    int flags;
 
-    /* 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)
-    {
-        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;
+    CLEAR(gw);
 
-    for (nh = (struct nlmsghdr *)rtbuf;
-         NLMSG_OK(nh, ssize);
-         nh = NLMSG_NEXT(nh, ssize))
+    if (net_route_v6_best_gw(ctx, dest, 0, &rgi6->gateway.addr_ipv6,
+                             rgi6->iface) == 0)
     {
-        struct rtmsg *rtm;
-        int attrlen;
-
-        if (nh->nlmsg_type == NLMSG_DONE)
+        if (rgi6->gateway.addr_ipv6.s6_addr)
         {
-            break;
+            rgi6->flags |= RGI_ADDR_DEFINED;
         }
 
-        if (nh->nlmsg_type == NLMSG_ERROR)
+        if (rgi6->iface)
         {
-            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;
-        }
-
-        if (nh->nlmsg_type != RTM_NEWROUTE)
-        {
-            /* 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,