[Openvpn-devel] cleanup open_tun() for TARGET_NETBSD

- 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(-)

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)
>       {

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

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

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