diff mbox series

[Openvpn-devel] FreeBSD-DCO: repair device iteration to find first free interface.

Message ID 20220819182439.71531-1-gert@greenie.muc.de
State Accepted
Headers
Series [Openvpn-devel] FreeBSD-DCO: repair device iteration to find first free interface. |

Commit Message

Gert Doering Aug. 19, 2022, 8:24 a.m. UTC 
  During review/update phase, FreeBSD/DCO's ability to find the first
free tun interface on "--dev tun" got broken, due to two issues:

 - create_interface() called msg(M_ERR|...), which is a fatal error
   and aborts OpenVPN, so "no retry with 'tun1' after 'tun0' failed"

   Change to M_WARN|M_ERRNO (= warning level, add strerror(errno), return).

 - open_tun_dco_generic() expects "-errno" as return value of
   open_tun_dco(), and breaks the loop on -EPERM.  create_interface()
   was returning "-1" instead (ioctl() error signalling), which happens
   to be "-EPERM" on FreeBSD.

   Change create_interface() to return -errno.

While at it, remove logging of errors from dco_freebsd.c::open_tun_dco()
(because all errors from create_interface() would be already logged there),
reducing open_tun_dco() to just a wrapper around create_interface().

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/dco_freebsd.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Comments

Kristof Provost Aug. 19, 2022, 10:27 p.m. UTC | #1 
Acked-by: Kristof Provost <kprovost@netgate.com>

Thanks!

Kristof

On 19 Aug 2022, at 20:24, Gert Doering wrote:
> During review/update phase, FreeBSD/DCO's ability to find the first
> free tun interface on "--dev tun" got broken, due to two issues:
>
>  - create_interface() called msg(M_ERR|...), which is a fatal error
>    and aborts OpenVPN, so "no retry with 'tun1' after 'tun0' failed"
>
>    Change to M_WARN|M_ERRNO (= warning level, add strerror(errno), return).
>
>  - open_tun_dco_generic() expects "-errno" as return value of
>    open_tun_dco(), and breaks the loop on -EPERM.  create_interface()
>    was returning "-1" instead (ioctl() error signalling), which happens
>    to be "-EPERM" on FreeBSD.
>
>    Change create_interface() to return -errno.
>
> While at it, remove logging of errors from dco_freebsd.c::open_tun_dco()
> (because all errors from create_interface() would be already logged there),
> reducing open_tun_dco() to just a wrapper around create_interface().
>
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  src/openvpn/dco_freebsd.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
> index 06b4d6a9..c6da6ce3 100644
> --- a/src/openvpn/dco_freebsd.c
> +++ b/src/openvpn/dco_freebsd.c
> @@ -178,7 +178,8 @@ create_interface(struct tuntap *tt, const char *dev)
>      ret = ioctl(tt->dco.fd, SIOCIFCREATE2, &ifr);
>      if (ret)
>      {
> -        msg(M_ERR | M_ERRNO, "Failed to create interface %s", ifr.ifr_name);
> +        ret = -errno;
> +        msg(M_WARN|M_ERRNO, "Failed to create interface %s (SIOCIFCREATE2)", ifr.ifr_name);
>          return ret;
>      }
>
> @@ -194,9 +195,10 @@ create_interface(struct tuntap *tt, const char *dev)
>      ret = ioctl(tt->dco.fd, SIOCSIFNAME, &ifr);
>      if (ret)
>      {
> +        ret = -errno;
>          /* Delete the created interface again. */
>          (void)ioctl(tt->dco.fd, SIOCIFDESTROY, &ifr);
> -        msg(M_ERR | M_ERRNO, "Failed to create interface %s", ifr.ifr_data);
> +        msg(M_WARN|M_ERRNO, "Failed to create interface %s (SIOCSIFNAME)", ifr.ifr_data);
>          return ret;
>      }
>
> @@ -229,16 +231,7 @@ remove_interface(struct tuntap *tt)
>  int
>  open_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx, const char *dev)
>  {
> -    int ret;
> -
> -    ret = create_interface(tt, dev);
> -
> -    if (ret < 0)
> -    {
> -        msg(M_ERR, "Failed to create interface");
> -    }
> -
> -    return ret;
> +    return create_interface(tt, dev);
>  }
>
>  void
> -- 
> 2.37.1
Gert Doering Aug. 19, 2022, 11:20 p.m. UTC | #2 
To avoid any fat fingers on my side, I've merged this and then subjected
the resulting branch to a round of FreeBSD DCO server tests, which all
behaved quite nicely :-)  (iroute missing, because not merged yet, but
the rest works - TCP [no DCO], UDP [DCO, if topology subnet], etc)

Aug 20 11:18:59 fbsd14 tun-udp-p2mp[79256]: GLOBAL_STATS,dco_enabled,1
Aug 20 11:18:59 fbsd14 tun-udp-p2mp-topology-subnet[79278]: GLOBAL_STATS,dco_enabled,1

Patch has been applied to the master branch.

commit efebdfe2de8dd5125fb42d646d03a958b7dc3eea
Author: Gert Doering
Date:   Fri Aug 19 20:24:39 2022 +0200

     FreeBSD-DCO: repair device iteration to find first free interface.

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Kristof Provost <kprovost@netgate.com>
     Message-Id: <20220819182439.71531-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25034.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 06b4d6a9..c6da6ce3 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -178,7 +178,8 @@  create_interface(struct tuntap *tt, const char *dev)
     ret = ioctl(tt->dco.fd, SIOCIFCREATE2, &ifr);
     if (ret)
     {
-        msg(M_ERR | M_ERRNO, "Failed to create interface %s", ifr.ifr_name);
+        ret = -errno;
+        msg(M_WARN|M_ERRNO, "Failed to create interface %s (SIOCIFCREATE2)", ifr.ifr_name);
         return ret;
     }
 
@@ -194,9 +195,10 @@  create_interface(struct tuntap *tt, const char *dev)
     ret = ioctl(tt->dco.fd, SIOCSIFNAME, &ifr);
     if (ret)
     {
+        ret = -errno;
         /* Delete the created interface again. */
         (void)ioctl(tt->dco.fd, SIOCIFDESTROY, &ifr);
-        msg(M_ERR | M_ERRNO, "Failed to create interface %s", ifr.ifr_data);
+        msg(M_WARN|M_ERRNO, "Failed to create interface %s (SIOCSIFNAME)", ifr.ifr_data);
         return ret;
     }
 
@@ -229,16 +231,7 @@  remove_interface(struct tuntap *tt)
 int
 open_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx, const char *dev)
 {
-    int ret;
-
-    ret = create_interface(tt, dev);
-
-    if (ret < 0)
-    {
-        msg(M_ERR, "Failed to create interface");
-    }
-
-    return ret;
+    return create_interface(tt, dev);
 }
 
 void