[Openvpn-devel] Include utun device number in utun error messages

Message ID 20200725235023.22441-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel] Include utun device number in utun error messages
Related show

Commit Message

Arne Schwabe July 25, 2020, 11:50 p.m.
For lack of a better API (or knowledge about a better API) we try to
open utun devices on macOS by trying utun0 to utun255 and use the
first one that works. On my Mac I have already 4 devices that
do nothing but are just there and another VPN connection resulting in a
number of error messages. This explicitly  shows in the log that we
tried the devices instead of some unspecific error.

This changes the log from:

Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opened utun device utun5

to

Opening utun0 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opening utun1 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opening utun2 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opening utun3 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opening utun4 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
Opened utun device utun5

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/tun.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

Comments

Jonathan K. Bullard July 26, 2020, 12:31 a.m. | #1
Hi,

On Sat, Jul 25, 2020 at 7:51 PM Arne Schwabe <arne@rfc2549.org> wrote:
>
> For lack of a better API (or knowledge about a better API) we try to
> open utun devices on macOS by trying utun0 to utun255 and use the
> first one that works. On my Mac I have already 4 devices that
> do nothing but are just there and another VPN connection resulting in a
> number of error messages. This explicitly  shows in the log that we
> tried the devices instead of some unspecific error.
>
> This changes the log from:
>
> Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opening utun (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opened utun device utun5
>
> to
>
> Opening utun0 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opening utun1 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opening utun2 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opening utun3 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opening utun4 failed (connect(AF_SYS_CONTROL)): Resource busy (errno=16)
> Opened utun device utun5

Feature-ACK. The failure messages have concerned some Tunnelblick
users. This _might_ help clarify things for them and it certainly
won't hurt.

I have not tested the code, but it looks fine.

Note that the last half of the patch consists only of whitespace
changes (starting at "@@ -5682,15 +5685,15 @@).

Note also that macOS itself creates some utun devices, which is why
the failures to open happen even on a clean install of macOS and only
OpenVPN. The number created depends on the version of macOS and the
Apple services such as iCloud Drive and Screen Sharing that are
enabled.

Best regards,

Jon Bullard
Arne Schwabe July 26, 2020, 12:38 a.m. | #2
> 
> Feature-ACK. The failure messages have concerned some Tunnelblick
> users. This _might_ help clarify things for them and it certainly
> won't hurt.
> 
> I have not tested the code, but it looks fine.
> 
> Note that the last half of the patch consists only of whitespace
> changes (starting at "@@ -5682,15 +5685,15 @@).

Yes my mistake, I did a uncrustify on the file before checking in but
did not double check that it only modified the parts that I modified before.

Arne
Gert Doering July 26, 2020, 1:40 p.m. | #3
Acked-by: Gert Doering <gert@greenie.muc.de>

I've left out the stray uncrustify fixes - not that I disagree with
them, but they should go to their own patch.  Sorry for not cleaning
that up while merging the corresponding patch.

Code change looks good.  Not tested anything (only test compiled on MacOS).

I wonder if we could just skip print the error message for this
particular call (connect()) if the return is EBUSY?  Or maybe
"skip if utunnumber < 10 && EBUSY"?

Your patch has been applied to the master branch.

commit 1d86fae8740a6d025d550fdcb4aa13c755fc2e48
Author: Arne Schwabe
Date:   Sun Jul 26 01:50:23 2020 +0200

     Include utun device number in utun error messages

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


--
kind regards,

Gert Doering
Arne Schwabe July 26, 2020, 2:19 p.m. | #4
Am 26.07.20 um 15:40 schrieb Gert Doering:
> Acked-by: Gert Doering <gert@greenie.muc.de>
> 
> I've left out the stray uncrustify fixes - not that I disagree with
> them, but they should go to their own patch.  Sorry for not cleaning
> that up while merging the corresponding patch.
> 
> Code change looks good.  Not tested anything (only test compiled on MacOS).
> 
> I wonder if we could just skip print the error message for this
> particular call (connect()) if the return is EBUSY?  Or maybe
> "skip if utunnumber < 10 && EBUSY"?
> 

since you can also specify that you want utun9 that would break that.
More realistically we should just skip the interfaces that are already
present in the system.  Baiscally if 'does interface utunX exist?' =>
skip on trying the first working one.

Arne
Gert Doering July 26, 2020, 2:38 p.m. | #5
Hi,

On Sun, Jul 26, 2020 at 04:19:21PM +0200, Arne Schwabe wrote:
> Am 26.07.20 um 15:40 schrieb Gert Doering:
> > I wonder if we could just skip print the error message for this
> > particular call (connect()) if the return is EBUSY?  Or maybe
> > "skip if utunnumber < 10 && EBUSY"?
> 
> since you can also specify that you want utun9 that would break that.

Only for the "loop from 0 to find the first working one", of course :)

> More realistically we should just skip the interfaces that are already
> present in the system.  Baiscally if 'does interface utunX exist?' =>
> skip on trying the first working one.

Yes that was the idea.

gert

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 82d96927..ed00644c 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -2950,14 +2950,16 @@  utun_open_helper(struct ctl_info ctlInfo, int utunnum)
 
     if (fd < 0)
     {
-        msg(M_INFO | M_ERRNO, "Opening utun (socket(SYSPROTO_CONTROL))");
+        msg(M_INFO | M_ERRNO, "Opening utun%d failed (socket(SYSPROTO_CONTROL))",
+            utunnum);
         return -2;
     }
 
     if (ioctl(fd, CTLIOCGINFO, &ctlInfo) == -1)
     {
         close(fd);
-        msg(M_INFO | M_ERRNO, "Opening utun (ioctl(CTLIOCGINFO))");
+        msg(M_INFO | M_ERRNO, "Opening utun%d failed (ioctl(CTLIOCGINFO))",
+            utunnum);
         return -2;
     }
 
@@ -2975,7 +2977,8 @@  utun_open_helper(struct ctl_info ctlInfo, int utunnum)
 
     if (connect(fd, (struct sockaddr *)&sc, sizeof(sc)) < 0)
     {
-        msg(M_INFO | M_ERRNO, "Opening utun (connect(AF_SYS_CONTROL))");
+        msg(M_INFO | M_ERRNO, "Opening utun%d failed (connect(AF_SYS_CONTROL))",
+            utunnum);
         close(fd);
         return -1;
     }
@@ -5682,15 +5685,15 @@  write_dhcp_str(struct buffer *buf, const int type, const char *str, bool *error)
  *  0x1D  0x7 openvpn 0x3 net 0x00 0x0A duckduckgo 0x3 com 0x00
  */
 static void
-write_dhcp_search_str(struct buffer *buf, const int type, const char * const *str_array,
+write_dhcp_search_str(struct buffer *buf, const int type, const char *const *str_array,
                       int array_len, bool *error)
 {
-    char         tmp_buf[256];
-    int          i;
-    int          len = 0;
-    int          label_length_pos;
+    char tmp_buf[256];
+    int i;
+    int len = 0;
+    int label_length_pos;
 
-    for (i=0; i < array_len; i++)
+    for (i = 0; i < array_len; i++)
     {
         const char  *ptr = str_array[i];
 
@@ -5701,7 +5704,7 @@  write_dhcp_search_str(struct buffer *buf, const int type, const char * const *st
             return;
         }
         /* Loop over all subdomains separated by a dot and replace the dot
-           with the length of the subdomain */
+         * with the length of the subdomain */
 
         /* label_length_pos points to the byte to be replaced by the length
          * of the following domain label */
@@ -5709,7 +5712,7 @@  write_dhcp_search_str(struct buffer *buf, const int type, const char * const *st
 
         while (true)
         {
-            if (*ptr == '.' || *ptr == '\0' )
+            if (*ptr == '.' || *ptr == '\0')
             {
                 tmp_buf[label_length_pos] = (len-label_length_pos)-1;
                 label_length_pos = len;
@@ -5769,8 +5772,8 @@  build_dhcp_options_string(struct buffer *buf, const struct tuntap_options *o)
     if (o->domain_search_list_len > 0)
     {
         write_dhcp_search_str(buf, 119, o->domain_search_list,
-                                        o->domain_search_list_len,
-                                       &error);
+                              o->domain_search_list_len,
+                              &error);
     }
 
     /* the MS DHCP server option 'Disable Netbios-over-TCP/IP
@@ -6149,9 +6152,9 @@  wintun_register_ring_buffer(struct tuntap *tt, const char *device_guid)
     else
     {
         msg(M_FATAL, "ERROR:  Wintun requires SYSTEM privileges and therefore "
-                     "should be used with interactive service. If you want to "
-                     "use openvpn from command line, you need to do SYSTEM "
-                     "elevation yourself (for example with psexec).");
+            "should be used with interactive service. If you want to "
+            "use openvpn from command line, you need to do SYSTEM "
+            "elevation yourself (for example with psexec).");
     }
 
     return ret;