[Openvpn-devel] cleanup open_tun() for TARGET_NETBSD

Message ID 20220808152344.17539-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel] cleanup open_tun() for TARGET_NETBSD | expand

Commit Message

Gert Doering Aug. 8, 2022, 3:23 p.m. UTC
- NetBSD "dynamic tap" (--dev tap -> tap<number>) handling had special
  #ifdef'ed code inside open_tun_generic() - pull out, move to NetBSD
  open_tun().  Roughly the same amount of code, less #ifdef, code flow
  is more clear.

- fix one spurious warning about "remote" not being initialized

- adjust NetBSD do_open() comments to actual code - the "pre NetBSD 4.0"
  code has long be removed, but the comment was still there.

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/tun.c | 72 +++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 34 deletions(-)

Comments

Antonio Quartulli Sept. 13, 2022, 9:33 p.m. UTC | #1
Hi,

On 08/08/2022 17:23, Gert Doering wrote:
> - NetBSD "dynamic tap" (--dev tap -> tap<number>) handling had special
>    #ifdef'ed code inside open_tun_generic() - pull out, move to NetBSD
>    open_tun().  Roughly the same amount of code, less #ifdef, code flow
>    is more clear.
> 
> - fix one spurious warning about "remote" not being initialized
> 
> - adjust NetBSD do_open() comments to actual code - the "pre NetBSD 4.0"
>    code has long be removed, but the comment was still there.
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>   src/openvpn/tun.c | 72 +++++++++++++++++++++++++----------------------
>   1 file changed, 38 insertions(+), 34 deletions(-)
> 
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 54f7d72c..9cba1c3a 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -1380,7 +1380,7 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,
>       }
>   
>   #elif defined(TARGET_NETBSD)
> -    in_addr_t remote_end;           /* for "virtual" subnet topology */
> +    in_addr_t remote_end = INADDR_ANY;	/* for "virtual" subnet topology */
>   
>       if (tun)
>       {
> @@ -1762,29 +1762,6 @@ open_tun_generic(const char *dev, const char *dev_type, const char *dev_node,
>                * explicit unit number.  Try opening /dev/[dev]n
>                * where n = [0, 255].
>                */
> -#ifdef TARGET_NETBSD
> -            /* on NetBSD, tap (but not tun) devices are opened by
> -             * opening /dev/tap and then querying the system about the
> -             * actual device name (tap0, tap1, ...) assigned
> -             */
> -            if (strcmp(dev, "tap") == 0)
> -            {
> -                struct ifreq ifr;
> -                if ((tt->fd = open( "/dev/tap", O_RDWR)) < 0)
> -                {
> -                    msg(M_FATAL, "Cannot allocate NetBSD TAP dev dynamically");
> -                }
> -                if (ioctl( tt->fd, TAPGIFNAME, (void *)&ifr ) < 0)
> -                {
> -                    msg(M_FATAL, "Cannot query NetBSD TAP device name");
> -                }
> -                CLEAR(dynamic_name);
> -                strncpy( dynamic_name, ifr.ifr_name, sizeof(dynamic_name)-1 );
> -                dynamic_opened = true;
> -                openvpn_snprintf(tunname, sizeof(tunname), "/dev/%s", dynamic_name );
> -            }
> -            else
> -#endif
>   
>               if (!tun_name_is_fixed(dev))
>               {
> @@ -2759,24 +2736,51 @@ read_tun(struct tuntap *tt, uint8_t *buf, int len)
>   #elif defined(TARGET_NETBSD)
>   
>   /*
> - * NetBSD before 4.0 does not support IPv6 on tun out of the box,
> - * but there exists a patch (sys/net/if_tun.c, 1.79->1.80, see PR 32944).
> - *
> - * NetBSD 4.0 and up do, but we need to put the tun interface into
> - * "multi_af" mode, which will prepend the address family to all packets
> - * (same as OpenBSD and FreeBSD).  If this is not enabled, the kernel
> - * silently drops all IPv6 packets on output and gets confused on input.
> + * NetBSD 4.0 and up support IPv6 on tun interfaces, but we need to put
> + * the tun interface into "multi_af" mode, which will prepend the address
> + * family to all packets (same as OpenBSD and FreeBSD).
>    *
> - * On earlier versions, multi_af is not available at all, so we have
> - * two different NetBSD code variants here :-(
> + * If this is not enabled, the kernel silently drops all IPv6 packets on
> + * output and gets confused on input.
>    *
> + * Note: --dev tap3 works *if* the interface is created externally by
> + *         "ifconfig tap3 create"
> + *         (and for devices beyond tap3, "mknod /dev/tapN c ...")
> + *       but we do not have code to do that inside OpenVPN
>    */
>   
>   void
>   open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tuntap *tt,
>            openvpn_net_ctx_t *ctx)
>   {
> -    open_tun_generic(dev, dev_type, dev_node, tt);
> +    /* on NetBSD, tap (but not tun) devices are opened by
> +     * opening /dev/tap and then querying the system about the
> +     * actual device name (tap0, tap1, ...) assigned
> +     */
> +    if (strcmp(dev, "tap") == 0)

in open_tun_generic() there is a check *before* reaching the code above:

"if (dev_node)"

After your patch this check becomes secondary and won't be reached if 
$dev == "tap".

Is this wanted? It sounds like dev_node is now ignored if $dev is tap.

Other than that the patch looks good and the new comment for open_tun is 
easy to digest, also for non bsders like me.

Cheers,

> +    {
> +        struct ifreq ifr;
> +        if ((tt->fd = open( "/dev/tap", O_RDWR)) < 0)
> +        {
> +            msg(M_FATAL, "Cannot allocate NetBSD TAP dev dynamically");
> +        }
> +        if (ioctl( tt->fd, TAPGIFNAME, (void *)&ifr ) < 0)
> +        {
> +            msg(M_FATAL, "Cannot query NetBSD TAP device name");
> +        }
> +        set_nonblock(tt->fd);
> +        set_cloexec(tt->fd); /* don't pass fd to scripts */
> +        msg(M_INFO, "TUN/TAP device %s opened", ifr.ifr_name);
> +
> +        tt->actual_name = string_alloc(ifr.ifr_name, NULL);
> +    }
> +    else
> +    {
> +        /* dynamic / named tun can be handled by the generic function
> +         * named tap ("tap3") is handled there as well, if pre-created
> +         */
> +        open_tun_generic(dev, dev_type, dev_node, tt);
> +    }
>   
>       if (tt->fd >= 0)
>       {
Gert Doering Sept. 14, 2022, 6:15 a.m. UTC | #2
Hi,

On Tue, Sep 13, 2022 at 11:33:20PM +0200, Antonio Quartulli wrote:
> in open_tun_generic() there is a check *before* reaching the code above:
> 
> "if (dev_node)"
> 
> After your patch this check becomes secondary and won't be reached if 
> $dev == "tap".
> 
> Is this wanted? It sounds like dev_node is now ignored if $dev is tap.

--dev-node on "not windows, not macos, not linux" is very ill-specified,
in other words, it usually doesn't do anything useful at all.

Basically,

 --dev-node tun3

would do exactly the same as "--dev tun3", for "tunN" devices
(where /dev/tunN exists and can be open()ed).

For tap, NetBSD does not support "tap3", so --dev-node tap<anything>
will just produce an "cannot open /dev/tap<anythign>" error message
(and even if there were a "new and improved tap driver", so you'd
want "--dev-node newtap" it would still not work as the code wouldn't
do the right "give me a dynamic device dance").

So yes, with the new code, --dev-node would be ignored for tap - which
was not actively intentional, but since there is really no way --dev-node
can be useful on NetBSD and --dev-type tap, I can live with that.


> Other than that the patch looks good and the new comment for open_tun is 
> easy to digest, also for non bsders like me.

Would that be an ACK? :-)

gert
Antonio Quartulli Sept. 14, 2022, 6:56 a.m. UTC | #3
Hi,

On 14/09/2022 08:15, Gert Doering wrote:
>> Other than that the patch looks good and the new comment for open_tun is
>> easy to digest, also for non bsders like me.
> 
> Would that be an ACK? :-)

Yes:

Acked-by: Antonio Quartulli <a@unstable.cc>

we may really want to prune this --dev-node thing once and for all on 
those platforms where it does nothing :)

Cheers,

> 
> gert
Gert Doering Sept. 14, 2022, 8:12 a.m. UTC | #4
Patch has been applied to the master branch.

One whitespace error fixed (tab before end-of-line comment) that uncrustify
complained about on merge.

commit 6e3fc642b74180f0e8e7ef63c0d6ca4e0c5537f4
Author: Gert Doering
Date:   Mon Aug 8 17:23:44 2022 +0200

     cleanup open_tun() for TARGET_NETBSD

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20220808152344.17539-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24849.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 54f7d72c..9cba1c3a 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1380,7 +1380,7 @@  do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,
     }
 
 #elif defined(TARGET_NETBSD)
-    in_addr_t remote_end;           /* for "virtual" subnet topology */
+    in_addr_t remote_end = INADDR_ANY;	/* for "virtual" subnet topology */
 
     if (tun)
     {
@@ -1762,29 +1762,6 @@  open_tun_generic(const char *dev, const char *dev_type, const char *dev_node,
              * explicit unit number.  Try opening /dev/[dev]n
              * where n = [0, 255].
              */
-#ifdef TARGET_NETBSD
-            /* on NetBSD, tap (but not tun) devices are opened by
-             * opening /dev/tap and then querying the system about the
-             * actual device name (tap0, tap1, ...) assigned
-             */
-            if (strcmp(dev, "tap") == 0)
-            {
-                struct ifreq ifr;
-                if ((tt->fd = open( "/dev/tap", O_RDWR)) < 0)
-                {
-                    msg(M_FATAL, "Cannot allocate NetBSD TAP dev dynamically");
-                }
-                if (ioctl( tt->fd, TAPGIFNAME, (void *)&ifr ) < 0)
-                {
-                    msg(M_FATAL, "Cannot query NetBSD TAP device name");
-                }
-                CLEAR(dynamic_name);
-                strncpy( dynamic_name, ifr.ifr_name, sizeof(dynamic_name)-1 );
-                dynamic_opened = true;
-                openvpn_snprintf(tunname, sizeof(tunname), "/dev/%s", dynamic_name );
-            }
-            else
-#endif
 
             if (!tun_name_is_fixed(dev))
             {
@@ -2759,24 +2736,51 @@  read_tun(struct tuntap *tt, uint8_t *buf, int len)
 #elif defined(TARGET_NETBSD)
 
 /*
- * NetBSD before 4.0 does not support IPv6 on tun out of the box,
- * but there exists a patch (sys/net/if_tun.c, 1.79->1.80, see PR 32944).
- *
- * NetBSD 4.0 and up do, but we need to put the tun interface into
- * "multi_af" mode, which will prepend the address family to all packets
- * (same as OpenBSD and FreeBSD).  If this is not enabled, the kernel
- * silently drops all IPv6 packets on output and gets confused on input.
+ * NetBSD 4.0 and up support IPv6 on tun interfaces, but we need to put
+ * the tun interface into "multi_af" mode, which will prepend the address
+ * family to all packets (same as OpenBSD and FreeBSD).
  *
- * On earlier versions, multi_af is not available at all, so we have
- * two different NetBSD code variants here :-(
+ * If this is not enabled, the kernel silently drops all IPv6 packets on
+ * output and gets confused on input.
  *
+ * Note: --dev tap3 works *if* the interface is created externally by
+ *         "ifconfig tap3 create"
+ *         (and for devices beyond tap3, "mknod /dev/tapN c ...")
+ *       but we do not have code to do that inside OpenVPN
  */
 
 void
 open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tuntap *tt,
          openvpn_net_ctx_t *ctx)
 {
-    open_tun_generic(dev, dev_type, dev_node, tt);
+    /* on NetBSD, tap (but not tun) devices are opened by
+     * opening /dev/tap and then querying the system about the
+     * actual device name (tap0, tap1, ...) assigned
+     */
+    if (strcmp(dev, "tap") == 0)
+    {
+        struct ifreq ifr;
+        if ((tt->fd = open( "/dev/tap", O_RDWR)) < 0)
+        {
+            msg(M_FATAL, "Cannot allocate NetBSD TAP dev dynamically");
+        }
+        if (ioctl( tt->fd, TAPGIFNAME, (void *)&ifr ) < 0)
+        {
+            msg(M_FATAL, "Cannot query NetBSD TAP device name");
+        }
+        set_nonblock(tt->fd);
+        set_cloexec(tt->fd); /* don't pass fd to scripts */
+        msg(M_INFO, "TUN/TAP device %s opened", ifr.ifr_name);
+
+        tt->actual_name = string_alloc(ifr.ifr_name, NULL);
+    }
+    else
+    {
+        /* dynamic / named tun can be handled by the generic function
+         * named tap ("tap3") is handled there as well, if pre-created
+         */
+        open_tun_generic(dev, dev_type, dev_node, tt);
+    }
 
     if (tt->fd >= 0)
     {