[Openvpn-devel,v8] tun: use is_tun_p2p more consistently

Message ID 20240906162514.78671-1-frank@lichtenheld.com
State Accepted
Headers show
Series [Openvpn-devel,v8] tun: use is_tun_p2p more consistently | expand

Commit Message

Frank Lichtenheld Sept. 6, 2024, 4:25 p.m. UTC
Using "tun" as the variable name for the return of
is_tun_p2p is probably a historical accident. But
it has actual consequences in that the other code
often seems to assume that it does less checks
than it actually does.

Use "tun_p2p" as the variable name and remove checks
that are not required. Also use is_tun_p2p in more
places.

Change-Id: Ice8b95f953c3f7e71657a78ea12b02a08c60aa67
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/380
This mail reflects revision 8 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>

Comments

Gert Doering Sept. 9, 2024, 9:33 a.m. UTC | #1
Sorry for taking so long to handle this.  These old code ruins make
my head spin...

Anyway.  Arne has ACKed it, GHA likes it, my server and client test rigs
find nothing to complain.

Did stare-at-code while the tests were running... all reasonable
changes, nothing should have a behavioural change (except the fix in
ifconfig_sanity_check()).  Good catch on print_topology() ;-)

Quite a few of the old
  "else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)"
are my doing, I think... seems "if (tun)" was really very unclear...

Also thanks for restructuring the Darwin section to be "just as all
the other platforms" :-)

Your patch has been applied to the master branch.

commit 976a65346d2193181f4f5664f798e16fcbf43345
Author: Frank Lichtenheld
Date:   Fri Sep 6 18:25:14 2024 +0200

     tun: use is_tun_p2p more consistently

     Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
     Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Message-Id: <20240906162514.78671-1-frank@lichtenheld.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg29091.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index ce3d882..739e008 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -499,31 +499,31 @@ 
 static const char ifconfig_warn_how_to_silence[] = "(silence this warning with --ifconfig-nowarn)";
 
 /*
- * If !tun, make sure ifconfig_remote_netmask looks
+ * If !tun_p2p, make sure ifconfig_remote_netmask looks
  *  like a netmask.
  *
- * If tun, make sure ifconfig_remote_netmask looks
+ * If tun_p2p, make sure ifconfig_remote_netmask looks
  *  like an IPv4 address.
  */
 static void
-ifconfig_sanity_check(bool tun, in_addr_t addr, int topology)
+ifconfig_sanity_check(bool tun_p2p, in_addr_t addr)
 {
     struct gc_arena gc = gc_new();
     const bool looks_like_netmask = ((addr & 0xFF000000) == 0xFF000000);
-    if (tun)
+    if (tun_p2p)
     {
-        if (looks_like_netmask && (topology == TOP_NET30 || topology == TOP_P2P))
+        if (looks_like_netmask)
         {
             msg(M_WARN, "WARNING: Since you are using --dev tun with a point-to-point topology, the second argument to --ifconfig must be an IP address.  You are using something (%s) that looks more like a netmask. %s",
                 print_in_addr_t(addr, 0, &gc),
                 ifconfig_warn_how_to_silence);
         }
     }
-    else /* tap */
+    else
     {
         if (!looks_like_netmask)
         {
-            msg(M_WARN, "WARNING: Since you are using --dev tap, the second argument to --ifconfig must be a netmask, for example something like 255.255.255.0. %s",
+            msg(M_WARN, "WARNING: Since you are using subnet topology, the second argument to --ifconfig must be a netmask, for example something like 255.255.255.0. %s",
                 ifconfig_warn_how_to_silence);
         }
     }
@@ -667,13 +667,13 @@ 
     struct buffer out = alloc_buf_gc(256, gc);
     if (tt->did_ifconfig_setup && !disable)
     {
-        if (tt->type == DEV_TYPE_TAP || (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET))
+        if (!is_tun_p2p(tt))
         {
             buf_printf(&out, "%s %s",
                        print_in_addr_t(tt->local & tt->remote_netmask, 0, gc),
                        print_in_addr_t(tt->remote_netmask, 0, gc));
         }
-        else if (tt->type == DEV_TYPE_TUN)
+        else if (tt->type == DEV_TYPE_TUN) /* tun p2p topology */
         {
             const char *l, *r;
             if (remote)
@@ -737,24 +737,24 @@ 
 bool
 is_tun_p2p(const struct tuntap *tt)
 {
-    bool tun = false;
+    bool tun_p2p = false;
 
     if (tt->type == DEV_TYPE_TAP
         || (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         || tt->type == DEV_TYPE_NULL)
     {
-        tun = false;
+        tun_p2p = false;
     }
     else if (tt->type == DEV_TYPE_TUN)
     {
-        tun = true;
+        tun_p2p = true;
     }
     else
     {
         msg(M_FATAL, "Error: problem with tun vs. tap setting"); /* JYFIXME -- needs to be caught earlier, in init_tun? */
 
     }
-    return tun;
+    return tun_p2p;
 }
 
 /*
@@ -831,12 +831,10 @@ 
 
     if (ifconfig_local_parm && ifconfig_remote_netmask_parm)
     {
-        bool tun = false;
-
         /*
          * We only handle TUN/TAP devices here, not --dev null devices.
          */
-        tun = is_tun_p2p(tt);
+        bool tun_p2p = is_tun_p2p(tt);
 
         /*
          * Convert arguments to binary IPv4 addresses.
@@ -853,7 +851,7 @@ 
             NULL);
 
         tt->remote_netmask = getaddr(
-            (tun ? GETADDR_RESOLVE : 0)
+            (tun_p2p ? GETADDR_RESOLVE : 0)
             | GETADDR_HOST_ORDER
             | GETADDR_FATAL_ON_SIGNAL
             | GETADDR_FATAL,
@@ -868,7 +866,7 @@ 
         if (strict_warn)
         {
             struct addrinfo *curele;
-            ifconfig_sanity_check(tt->type == DEV_TYPE_TUN, tt->remote_netmask, tt->topology);
+            ifconfig_sanity_check(tun_p2p, tt->remote_netmask);
 
             /*
              * If local_public or remote_public addresses are defined,
@@ -899,11 +897,11 @@ 
                 }
             }
 
-            if (tt->type == DEV_TYPE_TAP || (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET))
+            if (!tun_p2p)
             {
                 check_subnet_conflict(tt->local, tt->remote_netmask, "TUN/TAP adapter");
             }
-            else if (tt->type == DEV_TYPE_TUN)
+            else
             {
                 check_subnet_conflict(tt->local, IPV4_NETMASK_HOST, "TUN/TAP adapter");
             }
@@ -914,7 +912,7 @@ 
          * Make sure that both ifconfig addresses are part of the
          * same .252 subnet.
          */
-        if (tun)
+        if (tun_p2p)
         {
             verify_255_255_255_252(tt->local, tt->remote_netmask);
             tt->adapter_netmask = ~3;
@@ -1297,7 +1295,7 @@ 
     /*
      * We only handle TUN/TAP devices here, not --dev null devices.
      */
-    bool tun = is_tun_p2p(tt);
+    bool tun_p2p = is_tun_p2p(tt);
 #endif
 
 #if !defined(TARGET_LINUX)
@@ -1324,7 +1322,7 @@ 
         msg(M_FATAL, "Linux can't bring %s up", ifname);
     }
 
-    if (tun)
+    if (tun_p2p)
     {
         if (net_addr_ptp_v4_add(ctx, ifname, &tt->local,
                                 &tt->remote_netmask) < 0)
@@ -1343,27 +1341,8 @@ 
 #elif defined(TARGET_ANDROID)
     char out[64];
 
-    char *top;
-    switch (tt->topology)
-    {
-        case TOP_NET30:
-            top = "net30";
-            break;
-
-        case TOP_P2P:
-            top = "p2p";
-            break;
-
-        case TOP_SUBNET:
-            top = "subnet";
-            break;
-
-        default:
-            top = "undef";
-    }
-
     snprintf(out, sizeof(out), "%s %s %d %s", ifconfig_local,
-             ifconfig_remote_netmask, tun_mtu, top);
+             ifconfig_remote_netmask, tun_mtu, print_topology(tt->topology));
     management_android_control(management, "IFCONFIG", out);
 
 #elif defined(TARGET_SOLARIS)
@@ -1372,7 +1351,7 @@ 
      *    ifconfig tun2 10.2.0.2 10.2.0.1 mtu 1450 up
      *    ifconfig tun2 netmask 255.255.255.255
      */
-    if (tun)
+    if (tun_p2p)
     {
         argv_printf(&argv, "%s %s %s %s mtu %d up", IFCONFIG_PATH, ifname,
                     ifconfig_local, ifconfig_remote_netmask, tun_mtu);
@@ -1386,13 +1365,13 @@ 
         argv_printf(&argv, "%s %s netmask 255.255.255.255", IFCONFIG_PATH,
                     ifname);
     }
-    else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
+    else if (tt->type == DEV_TYPE_TUN)
     {
         argv_printf(&argv, "%s %s %s %s netmask %s mtu %d up", IFCONFIG_PATH,
                     ifname, ifconfig_local, ifconfig_local,
                     ifconfig_remote_netmask, tun_mtu);
     }
-    else
+    else /* tap */
     {
         argv_printf(&argv, "%s %s %s netmask %s up",
                     IFCONFIG_PATH, ifname, ifconfig_local,
@@ -1405,7 +1384,7 @@ 
         solaris_error_close(tt, es, ifname, false);
     }
 
-    if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
+    if (!tun_p2p && tt->type == DEV_TYPE_TUN)
     {
         /* Add a network route for the local tun interface */
         struct route_ipv4 r;
@@ -1429,14 +1408,14 @@ 
      */
 
     /* example: ifconfig tun2 10.2.0.2 10.2.0.1 mtu 1450 netmask 255.255.255.255 up */
-    if (tun)
+    if (tun_p2p)
     {
         argv_printf(&argv,
                     "%s %s %s %s mtu %d netmask 255.255.255.255 up -link0",
                     IFCONFIG_PATH, ifname, ifconfig_local,
                     ifconfig_remote_netmask, tun_mtu);
     }
-    else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
+    else if (tt->type == DEV_TYPE_TUN)
     {
         remote_end = create_arbitrary_remote( tt );
         argv_printf(&argv, "%s %s %s %s mtu %d netmask %s up -link0",
@@ -1444,7 +1423,7 @@ 
                     print_in_addr_t(remote_end, 0, &gc), tun_mtu,
                     ifconfig_remote_netmask);
     }
-    else
+    else /* tap */
     {
         argv_printf(&argv, "%s %s %s netmask %s mtu %d link0",
                     IFCONFIG_PATH, ifname, ifconfig_local,
@@ -1454,7 +1433,7 @@ 
     openvpn_execve_check(&argv, es, S_FATAL, "OpenBSD ifconfig failed");
 
     /* Add a network route for the local tun interface */
-    if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
+    if (!tun_p2p && tt->type == DEV_TYPE_TUN)
     {
         struct route_ipv4 r;
         CLEAR(r);
@@ -1468,20 +1447,20 @@ 
 #elif defined(TARGET_NETBSD)
     in_addr_t remote_end = INADDR_ANY;  /* for "virtual" subnet topology */
 
-    if (tun)
+    if (tun_p2p)
     {
         argv_printf(&argv, "%s %s %s %s mtu %d netmask 255.255.255.255 up",
                     IFCONFIG_PATH, ifname, ifconfig_local,
                     ifconfig_remote_netmask, tun_mtu);
     }
-    else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
+    else if (tt->type == DEV_TYPE_TUN)
     {
         remote_end = create_arbitrary_remote(tt);
         argv_printf(&argv, "%s %s %s %s mtu %d netmask %s up", IFCONFIG_PATH,
                     ifname, ifconfig_local, print_in_addr_t(remote_end, 0, &gc),
                     tun_mtu, ifconfig_remote_netmask);
     }
-    else
+    else /* tap */
     {
         /*
          * NetBSD has distinct tun and tap devices
@@ -1496,7 +1475,7 @@ 
     openvpn_execve_check(&argv, es, S_FATAL, "NetBSD ifconfig failed");
 
     /* Add a network route for the local tun interface */
-    if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
+    if (!tun_p2p && tt->type == DEV_TYPE_TUN)
     {
         struct route_ipv4 r;
         CLEAR(r);
@@ -1520,33 +1499,30 @@ 
 
 
     /* example: ifconfig tun2 10.2.0.2 10.2.0.1 mtu 1450 netmask 255.255.255.255 up */
-    if (tun)
+    if (tun_p2p)
     {
         argv_printf(&argv, "%s %s %s %s mtu %d netmask 255.255.255.255 up",
                     IFCONFIG_PATH, ifname, ifconfig_local,
                     ifconfig_remote_netmask, tun_mtu);
     }
-    else
+    else if (tt->type == DEV_TYPE_TUN)
     {
-        if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
-        {
-            argv_printf(&argv, "%s %s %s %s netmask %s mtu %d up",
-                        IFCONFIG_PATH, ifname, ifconfig_local, ifconfig_local,
-                        ifconfig_remote_netmask, tun_mtu);
-        }
-        else
-        {
-            argv_printf(&argv, "%s %s %s netmask %s mtu %d up", IFCONFIG_PATH,
-                        ifname, ifconfig_local, ifconfig_remote_netmask,
-                        tun_mtu);
-        }
+        argv_printf(&argv, "%s %s %s %s netmask %s mtu %d up",
+                    IFCONFIG_PATH, ifname, ifconfig_local, ifconfig_local,
+                    ifconfig_remote_netmask, tun_mtu);
+    }
+    else /* tap */
+    {
+        argv_printf(&argv, "%s %s %s netmask %s mtu %d up", IFCONFIG_PATH,
+                    ifname, ifconfig_local, ifconfig_remote_netmask,
+                    tun_mtu);
     }
 
     argv_msg(M_INFO, &argv);
     openvpn_execve_check(&argv, es, S_FATAL, "Mac OS X ifconfig failed");
 
     /* Add a network route for the local tun interface */
-    if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
+    if (!tun_p2p && tt->type == DEV_TYPE_TUN)
     {
         struct route_ipv4 r;
         CLEAR(r);
@@ -1560,7 +1536,7 @@ 
 #elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)
 
     /* example: ifconfig tun2 10.2.0.2 10.2.0.1 mtu 1450 netmask 255.255.255.255 up */
-    if (tun)       /* point-to-point tun */
+    if (tun_p2p)    /* point-to-point tun */
     {
         argv_printf(&argv, "%s %s %s %s mtu %d netmask 255.255.255.255 up",
                     IFCONFIG_PATH, ifname, ifconfig_local,
@@ -1582,7 +1558,7 @@ 
         struct env_set *aix_es = env_set_create(NULL);
         env_set_add( aix_es, "ODMDIR=/etc/objrepos" );
 
-        if (tun)
+        if (tt->type == DEV_TYPE_TUN)
         {
             msg(M_FATAL, "no tun support on AIX (canthappen)");
         }