[Qemu-devel] [PATCH 0/2] net/tap-win32 bugfixes

[Qemu-devel] [PATCH 0/2] net/tap-win32 bugfixes

[Andrew Baumann]

Since I just discovered that you're in feature freeze, I wanted to
submit two smallish bug fixes for the win32 tap device. The changes
are independent, but without both fixes the tap device is unusable
(for me, at least!).

Cheers,
Andrew

[Qemu-devel] [PATCH 1/2] tap-win32: skip unexpected nodes during registry enumeration

[Andrew Baumann]

In order to find a named tap device, get_device_guid() enumerates children of
HKLM\SYSTEM\CCS\Control\Network\{4D36E972-E325-11CE-BFC1-08002BE10318}
(aka NETWORK_CONNECTIONS_KEY). For each child, it then looks for a
"Connection" subkey, but if this key doesn't exist, it aborts the
entire search. This was observed to fail on at least one Windows 10
machine, where there is an additional child of NETWORK_CONNECTIONS_KEY
(named "Descriptions"). Since registry enumeration doesn't guarantee
any particular sort order, we should continue to search for matching
children rather than aborting the search.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 net/tap-win32.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 4e2fa55..5e5d6db 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -356,7 +356,8 @@ static int get_device_guid(
                 &len);
 
             if (status != ERROR_SUCCESS || name_type != REG_SZ) {
-                    return -1;
+                ++i;
+                continue;
             }
             else {
                 if (is_tap_win32_dev(enum_name)) {
-- 
2.5.3

Re: [Qemu-devel] [PATCH 1/2] tap-win32: skip unexpected nodes during registry enumeration

[Stefan Weil]

Am 17.11.2015 um 20:09 schrieb Andrew Baumann:
> In order to find a named tap device, get_device_guid() enumerates children of
> HKLM\SYSTEM\CCS\Control\Network\{4D36E972-E325-11CE-BFC1-08002BE10318}
> (aka NETWORK_CONNECTIONS_KEY). For each child, it then looks for a
> "Connection" subkey, but if this key doesn't exist, it aborts the
> entire search. This was observed to fail on at least one Windows 10
> machine, where there is an additional child of NETWORK_CONNECTIONS_KEY
> (named "Descriptions"). Since registry enumeration doesn't guarantee
> any particular sort order, we should continue to search for matching
> children rather than aborting the search.
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
>  net/tap-win32.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 4e2fa55..5e5d6db 100644
> --- a/net/tap-win32.c
> +++ b/net/tap-win32.c
> @@ -356,7 +356,8 @@ static int get_device_guid(
>                  &len);
>  
>              if (status != ERROR_SUCCESS || name_type != REG_SZ) {
> -                    return -1;
> +                ++i;
> +                continue;
>              }
>              else {
>                  if (is_tap_win32_dev(enum_name)) {

Good catch!

Reviewed-by: Stefan Weil <sw@weilnetz.de>

[Qemu-devel] [PATCH 2/2] tap-win32: disable broken async write path

[Andrew Baumann]

The code under the TUN_ASYNCHRONOUS_WRITES path makes two incorrect
assumptions about the behaviour of the WriteFile API for overlapped
file handles. First, WriteFile does not update the
lpNumberOfBytesWritten parameter when the overlapped parameter is
non-NULL (the number of bytes written is known only when the operation
completes). Second, the buffer shouldn't be touched (or freed) until
the operation completes. This led to at least one bug where
tap_win32_write returned zero bytes written, which in turn caused
further writes ("receives") to be disabled for that device.

This change disables the asynchronous write path, while keeping most
of the code around in case someone sees value in resurrecting it. It
also adds some conditional debug output, similar to the read path.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
Although this version of the patch tries to be minimally invasive and
keep the code around, I'm not sure if there is much value in fixing
this -- I haven't checked, but would expect that the write to a tap
device is just a fairly fast buffer copy. If you would prefer a patch
that simply removes the async write support entirely, that should be
easy to do.

 net/tap-win32.c | 48 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 5e5d6db..02a3b0d 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -77,7 +77,12 @@
 
 //#define DEBUG_TAP_WIN32
 
-#define TUN_ASYNCHRONOUS_WRITES 1
+/* FIXME: The asynch write path appears to be broken at
+ * present. WriteFile() ignores the lpNumberOfBytesWritten parameter
+ * for overlapped writes, with the result we return zero bytes sent,
+ * and after handling a single packet, receive is disabled for this
+ * interface. */
+/* #define TUN_ASYNCHRONOUS_WRITES 1 */
 
 #define TUN_BUFFER_SIZE 1560
 #define TUN_MAX_BUFFER_COUNT 32
@@ -461,26 +466,47 @@ static int tap_win32_write(tap_win32_overlapped_t *overlapped,
     BOOL result;
     DWORD error;
 
+#ifdef TUN_ASYNCHRONOUS_WRITES
     result = GetOverlappedResult( overlapped->handle, &overlapped->write_overlapped,
                                   &write_size, FALSE);
 
     if (!result && GetLastError() == ERROR_IO_INCOMPLETE)
         WaitForSingleObject(overlapped->write_event, INFINITE);
+#endif
 
     result = WriteFile(overlapped->handle, buffer, size,
-                       &write_size, &overlapped->write_overlapped);
+                       NULL, &overlapped->write_overlapped);
+
+#ifdef TUN_ASYNCHRONOUS_WRITES
+    /* FIXME: we can't sensibly set write_size here, without waiting
+     * for the IO to complete! Moreover, we can't return zero,
+     * because that will disable receive on this interface, and we
+     * also can't assume it will succeed and return the full size,
+     * because that will result in the buffer being reclaimed while
+     * the IO is in progress. */
+#error Async writes are broken. Please disable TUN_ASYNCHRONOUS_WRITES.
+#else /* !TUN_ASYNCHRONOUS_WRITES */
+    if (!result) {
+        error = GetLastError();
+        if (error == ERROR_IO_PENDING) {
+            result = GetOverlappedResult(overlapped->handle,
+                                         &overlapped->write_overlapped,
+                                         &write_size, TRUE);
+        }
+    }
+#endif
 
     if (!result) {
-        switch (error = GetLastError())
-        {
-        case ERROR_IO_PENDING:
-#ifndef TUN_ASYNCHRONOUS_WRITES
-            WaitForSingleObject(overlapped->write_event, INFINITE);
+#ifdef DEBUG_TAP_WIN32
+        LPVOID msgbuf;
+        error = GetLastError();
+        FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSAGE_FROM_SYSTEM,
+                      NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                      (LPTSTR)&msgbuf, 0, NULL);
+        fprintf(stderr, "Tap-Win32: Error WriteFile %d - %s\n", error, msgbuf);
+        LocalFree(msgbuf);
 #endif
-            break;
-        default:
-            return -1;
-        }
+        return 0;
     }
 
     return write_size;
-- 
2.5.3

Re: [Qemu-devel] [PATCH 2/2] tap-win32: disable broken async write path

[Stefan Weil]

Am 17.11.2015 um 20:09 schrieb Andrew Baumann:
> The code under the TUN_ASYNCHRONOUS_WRITES path makes two incorrect
> assumptions about the behaviour of the WriteFile API for overlapped
> file handles. First, WriteFile does not update the
> lpNumberOfBytesWritten parameter when the overlapped parameter is
> non-NULL (the number of bytes written is known only when the operation
> completes). Second, the buffer shouldn't be touched (or freed) until
> the operation completes. This led to at least one bug where
> tap_win32_write returned zero bytes written, which in turn caused
> further writes ("receives") to be disabled for that device.
>
> This change disables the asynchronous write path, while keeping most
> of the code around in case someone sees value in resurrecting it. It
> also adds some conditional debug output, similar to the read path.
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> Although this version of the patch tries to be minimally invasive and
> keep the code around, I'm not sure if there is much value in fixing
> this -- I haven't checked, but would expect that the write to a tap
> device is just a fairly fast buffer copy. If you would prefer a patch
> that simply removes the async write support entirely, that should be
> easy to do.
>
>  net/tap-win32.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 5e5d6db..02a3b0d 100644
> --- a/net/tap-win32.c
> +++ b/net/tap-win32.c
> @@ -77,7 +77,12 @@
>  
>  //#define DEBUG_TAP_WIN32
>  
> -#define TUN_ASYNCHRONOUS_WRITES 1
> +/* FIXME: The asynch write path appears to be broken at
> + * present. WriteFile() ignores the lpNumberOfBytesWritten parameter
> + * for overlapped writes, with the result we return zero bytes sent,
> + * and after handling a single packet, receive is disabled for this
> + * interface. */
> +/* #define TUN_ASYNCHRONOUS_WRITES 1 */
>  
>  #define TUN_BUFFER_SIZE 1560
>  #define TUN_MAX_BUFFER_COUNT 32
> @@ -461,26 +466,47 @@ static int tap_win32_write(tap_win32_overlapped_t *overlapped,
>      BOOL result;
>      DWORD error;
>  
> +#ifdef TUN_ASYNCHRONOUS_WRITES
>      result = GetOverlappedResult( overlapped->handle, &overlapped->write_overlapped,
>                                    &write_size, FALSE);
>  
>      if (!result && GetLastError() == ERROR_IO_INCOMPLETE)
>          WaitForSingleObject(overlapped->write_event, INFINITE);
> +#endif
>  
>      result = WriteFile(overlapped->handle, buffer, size,
> -                       &write_size, &overlapped->write_overlapped);
> +                       NULL, &overlapped->write_overlapped);
> +
> +#ifdef TUN_ASYNCHRONOUS_WRITES
> +    /* FIXME: we can't sensibly set write_size here, without waiting
> +     * for the IO to complete! Moreover, we can't return zero,
> +     * because that will disable receive on this interface, and we
> +     * also can't assume it will succeed and return the full size,
> +     * because that will result in the buffer being reclaimed while
> +     * the IO is in progress. */
> +#error Async writes are broken. Please disable TUN_ASYNCHRONOUS_WRITES.
> +#else /* !TUN_ASYNCHRONOUS_WRITES */
> +    if (!result) {
> +        error = GetLastError();
> +        if (error == ERROR_IO_PENDING) {
> +            result = GetOverlappedResult(overlapped->handle,
> +                                         &overlapped->write_overlapped,
> +                                         &write_size, TRUE);
> +        }
> +    }
> +#endif
>  
>      if (!result) {
> -        switch (error = GetLastError())
> -        {
> -        case ERROR_IO_PENDING:
> -#ifndef TUN_ASYNCHRONOUS_WRITES
> -            WaitForSingleObject(overlapped->write_event, INFINITE);
> +#ifdef DEBUG_TAP_WIN32
> +        LPVOID msgbuf;

Does this also work ...

    char *msgbuf;


> +        error = GetLastError();
> +        FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSAGE_FROM_SYSTEM,
> +                      NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +                      (LPTSTR)&msgbuf, 0, NULL);

... and remove the type cast here? If it works without compiler
warnings, I'd prefer that variant.

> +        fprintf(stderr, "Tap-Win32: Error WriteFile %d - %s\n", error, msgbuf);
> +        LocalFree(msgbuf);
>  #endif
> -            break;
> -        default:
> -            return -1;
> -        }
> +        return 0;
>      }
>  
>      return write_size;

Otherwise, the patch looks good, but I currently cannot test it.

Stefan

Re: [Qemu-devel] [PATCH 2/2] tap-win32: disable broken async write path

[Andrew Baumann]

From: Stefan Weil [mailto:sw@weilnetz.de]
Sent: Tuesday, 17 November 2015 23:40
> > +#ifdef DEBUG_TAP_WIN32
> > +        LPVOID msgbuf;
> 
> Does this also work ...
> 
>     char *msgbuf;
> 
> 
> > +        error = GetLastError();
> > +
> FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSA
> GE_FROM_SYSTEM,
> > +                      NULL, error, MAKELANGID(LANG_NEUTRAL,
> SUBLANG_DEFAULT),
> > +                      (LPTSTR)&msgbuf, 0, NULL);
> 
> ... and remove the type cast here? If it works without compiler
> warnings, I'd prefer that variant.

Thanks Stefan. I was able to change the declaration to LPTSTR and avoid the cast, but am reticent about changing it to a bare char *, because the definition of LPTSTR depends on #ifdef UNICODE. I also fixed a lurking bug I had accidentally introduced in the event that WriteFile completes synchronously.

Revised patch incoming shortly...

Andrew

Re: [Qemu-devel] [PATCH 0/2] net/tap-win32 bugfixes

[Jason Wang]

On 11/18/2015 03:09 AM, Andrew Baumann wrote:
> Since I just discovered that you're in feature freeze, I wanted to
> submit two smallish bug fixes for the win32 tap device. The changes
> are independent, but without both fixes the tap device is unusable
> (for me, at least!).
>
> Cheers,
> Andrew
>

The patches looks good, but I'm not familiar with Win32 API, so any
"Acked-by" or "Reviewed-by" is more than appreciated.

Will leave them on the list for several days before applying

Thanks.